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

Fix an "implicitly-declared copy assignment operator" warning #693

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ssilverman
Copy link
Contributor

@ssilverman ssilverman commented Mar 17, 2023

Discovered with -Wextra (-Wdeprecated-copy).

Addresses part of #660.

I think I've done this correctly, but please triple-check my changes. (@luni64 I'd love your opinion here too.) I believe the behaviour changes because, when it uses the implicitly-declared copy assignment operator, copy_tcd() isn't called, but with my changes, it is called.

It's this function that's complaining:

DMASetting(const DMASetting &c) {
  TCD = &tcddata;
  *this = c;  // <-- Over here
}

@ssilverman
Copy link
Contributor Author

Making this a draft for now.

@ssilverman ssilverman marked this pull request as draft March 17, 2023 19:33
Discovered with -Wextra (-Wdeprecated-copy).
@ssilverman ssilverman force-pushed the fix-deprecated-copy-warnings branch from 53507bf to 34ca0df Compare July 15, 2023 18:07
@ssilverman
Copy link
Contributor Author

This commit also makes sure each copy constructor has a sibling operator=().

@luni64
Copy link
Contributor

luni64 commented Jul 16, 2023

Here is my assessment as requested by @ssilverman:

By default, if not provided or deleted by the user, the compiler automatically generates both a copy constructor and the corresponding assignment operator. If the class has a user-defined copy constructor but no user-defined assignment operator, auto-generating the missing assignment operator is considered 'dangerous' and is deprecated.

This results in the following very explicit warning message:

.../DMAChannel.h: In copy constructor 'DMASetting::DMASetting(const DMASetting&)':
.../DMAChannel.h:524:25: warning: implicitly-declared 'DMASetting& DMASetting::operator=(const DMASetting&)' is deprecated [-Wdeprecated-copy]
  524 |                 *this = c;
      |                         ^
.../DMAChannel.h:521:9: note: because 'DMASetting' has user-provided 'DMASetting::DMASetting(const DMASetting&)'
  521 |         DMASetting(const DMASetting &c)
      |         ^~~~~~~~~~

Since deleting the assignment operator is not an option here, one should define a corresponding user assignment operator as @ssilverman proposes. I also doubt that the auto-generated assignment operator does perform a proper copy_tcd(), I assume it does some bit-wise copying of the data only.

@ssilverman ssilverman marked this pull request as ready for review August 26, 2023 21: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.

None yet

2 participants