Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

candidates_pending_availability added #2253

Conversation

ErakhtinB
Copy link
Contributor

@ErakhtinB ErakhtinB commented Oct 30, 2024

Referenced issues

Description of the Change

Possible Drawbacks

Checklist Before Opening a PR

Before you open a Pull Request (PR), please make sure you've completed the following steps and confirm by answering 'Yes' to each item:

  1. Code is formatted: Have you run your code through clang-format to ensure it adheres to the project's coding standards? [Yes|No]
  2. Code is documented: Have you added comments and documentation to your code according to the guidelines in the project's contributing guidelines? [Yes|No]
  3. Self-review: Have you reviewed your own code to ensure it is free of typos, syntax errors, logical errors, and unresolved TODOs or FIXME without linking to an issue? [Yes|No]
  4. Zombienet Tests: Have you ensured that the zombienet tests are passing? Zombienet is a network simulation and testing tool used in this project. It's important to ensure that these tests pass to maintain the stability and reliability of the project. [Yes|No]

@ErakhtinB ErakhtinB requested review from kamilsa and Harrm October 30, 2024 06:48
* @when candidates_pending_availability() is invoked
* @then successful result is returned
*/
TEST_F(ParachainHostTest, DISABLED_CandidatesPendingAvailabilityTest) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every test in this file is disabled

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why?

block,
"ParachainHost_candidates_pending_availability",
id));
return *ref;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the copy eliminated here/how large can this vector be? Maybe construct an outcome::result and move the vector there explicitly to avoid copying, if it can be large?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added explicit moving

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return std::move is typically a worse alternative, I mean construct a variable with type exactly matching the result type and move in its constructor, this practically guarantees RVO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think move constructor for return value is guaranteed by this move. outcome::result<std::vector<std::optional>> res = std::move(*ref) is literally what's gonna happen. Anyway the vector will be moved efficiently.

@ErakhtinB ErakhtinB enabled auto-merge (squash) November 29, 2024 12:13
@ErakhtinB ErakhtinB merged commit ced71b3 into master Nov 29, 2024
12 checks passed
@ErakhtinB ErakhtinB deleted the 42-add-support-for-new-runtime-api-function-candidates_pending_availability branch November 29, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants