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

Typedfield has bad move constructor #4567

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

HowardHinnant
Copy link
Contributor

@HowardHinnant HowardHinnant commented Jun 12, 2023

High Level Overview of Change

A minor cleanup in TypedField. It removes a non-working and unused move constructor. And it constrains the remaining constructor to not be overly generic enough as to be used as a copy or move constructor.

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

@HowardHinnant HowardHinnant changed the title Typed fieldnomove Typedfield has bad move constructor Jun 12, 2023
TypedField(TypedField&& u) : SField(std::move(u))
{
}
explicit TypedField(private_access_tag_t pat, Args&&... args);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need the explicit keyword? Won't the private_access_tag_t prevent the compiler from using implicit type conversion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not if the parameter pack is empty, which it likely never will be in practice, but explicit doesn't hurt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, that's crazy, i didn't know that! that pulls the rug under the purpose of having the private tag :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no real motivation to allow implicit construction as it is only constructed in one file, and even that is handled by a macro.

@intelliot intelliot requested a review from ckeshava June 23, 2023 05:15
@ckeshava
Copy link
Collaborator

The Unix unity build is failing, but I'm not sure why :/

@HowardHinnant
Copy link
Contributor Author

@ckeshava Can you paste in the first error message you're seeing?

@ckeshava
Copy link
Collaborator

I didn't see any issue on my Mac. I was going through the CI/CD pipeline and saw this error

@HowardHinnant HowardHinnant added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 24, 2023
@HowardHinnant
Copy link
Contributor Author

Rebased to develop.

@intelliot intelliot merged commit 9eb30d4 into XRPLF:develop Jun 26, 2023
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Jul 10, 2023
Apply a minor cleanup in `TypedField`:
* Remove a non-working and unused move constructor.
* Constrain the remaining constructor to not be overly generic enough as
  to be used as a copy or move constructor.
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
Apply a minor cleanup in `TypedField`:
* Remove a non-working and unused move constructor.
* Constrain the remaining constructor to not be overly generic enough as
  to be used as a copy or move constructor.
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
Apply a minor cleanup in `TypedField`:
* Remove a non-working and unused move constructor.
* Constrain the remaining constructor to not be overly generic enough as
  to be used as a copy or move constructor.
@wuying520
Copy link

Hello, when will Radar open the network? Please reply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants