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

Disallow copying of State #773

Merged
merged 2 commits into from Feb 21, 2020
Merged

Disallow copying of State #773

merged 2 commits into from Feb 21, 2020

Conversation

lkloh
Copy link
Contributor

@lkloh lkloh commented Jan 14, 2020

To support the coding guidelines that ban copying into functions.

RE: https://github.com/swift-nav/interleaver/pull/2/files#r366537476

@@ -37,6 +37,10 @@ class State {
IReader *reader_;
IWriter *writer_;

State(const State&);
Copy link
Contributor

@RReichert RReichert Jan 14, 2020

Choose a reason for hiding this comment

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

The code you've added doesn't actually achieve the goal of disabling the copying of the class, it just tells the compiler that there should be copy constructor and assignment constructor defined somewhere, in the code. The compiler won't realize that there is an error until it performs the linking state of the build process, which is one of the last steps. The best way to achieve the goal of disabling of a copy constructor/assignment is to do the following.

State(const State&) = delete;
State& operator=(const State&) = delete;

After you make these change, try and write a unit test to verify that it works (you won't be able to commit the work because it will error out the compiler, but at least it is a way to check that your code works).

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Eventually we might want to enable move operations, but disallowing copies should implicitly disallow moves for now.

Copy link
Contributor

@RReichert RReichert left a comment

Choose a reason for hiding this comment

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

LGTM

@jbangelo jbangelo merged commit 0c0c3c1 into master Feb 21, 2020
@jbangelo jbangelo deleted the lkloh_nocopy branch February 21, 2020 17:04
@lkloh
Copy link
Contributor Author

lkloh commented Feb 21, 2020

Thanks for finishing it off! Gonna merge the submodule updates since they passed

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.

None yet

3 participants