Page MenuHomePhabricator

Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra
ClosedPublic

Authored by poelmanc on Oct 17 2019, 3:16 PM.

Details

Summary

readability-redundant-member-init removes redundant / unnecessary member and base class initialization. Unfortunately for the specific case of a copy constructor's initialization of a base class, gcc at strict warning levels warns if "base class is not initialized in the copy constructor of a derived class".

This patch adds an option IgnoreBaseInCopyConstructors defaulting to 0 (thus maintaining current behavior by default) to skip the specific case of removal of redundant base class initialization in the copy constructor. Enabling this option enables the resulting code to continue to compile successfully under gcc -Werror=extra. New test cases WithCopyConstructor1 and WithCopyConstructor2 in clang-tools-extra/test/clang-tidy/readability-redundant-member-init.cpp show that it removes redundant members even from copy constructors.

Diff Detail

Event Timeline

poelmanc created this revision.Oct 17 2019, 3:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2019, 3:16 PM

Exactly due to the issue you are fixing here, we ended up disabling the complete check because we didn't want to live with the warnings it produced on -Wextra.
Therefore, I'm actually strongly in favor to enable the option by default.

When others see that clang-tidy fixits introduce warnings (with -Wextra) or even break their build (with -Werror), they might not look into check options, but just disable the check directly.

mgehre added inline comments.Oct 19 2019, 4:12 AM
clang-tools-extra/clang-tidy/readability/RedundantMemberInitCheck.cpp
56

Extra space around string literal - did you run clang-format on your changes?

clang-tools-extra/docs/clang-tidy/checks/readability-redundant-member-init.rst
30

I'd might be helpful to mention gcc and -Wextra here so users can easily identify whether this case applies to them.

So https://godbolt.org/z/qzjU-C
This feels like gcc being overly zealous, i'm not sure what it says with that warning.

Exactly due to the issue you are fixing here, we ended up disabling the complete check because we didn't want to live with the warnings it produced on -Wextra.
Therefore, I'm actually strongly in favor to enable the option by default.

When others see that clang-tidy fixits introduce warnings (with -Wextra) or even break their build (with -Werror), they might not look into check options, but just disable the check directly.

Thank you for the feedback! I certainly like your suggestion to enable this by default but will await a collective conclusion before changing that.

poelmanc updated this revision to Diff 225764.Oct 19 2019, 12:43 PM

Addressed @mgehre's feedback:

  • Ran clang-format
  • Included gcc, -Wextra, -Werror=extra, and the full text of the warning/error message to aid in searchability, so people can find this option when they encounter the problem. Also added example code to the documentation.

So https://godbolt.org/z/qzjU-C
This feels like gcc being overly zealous, i'm not sure what it says with that warning.

I'd agree, while copy constructors usually need to copy base class information as well, and forgetting to do so may indicate a programming error, this warning may be more appropriate as a linter suggestion than a compiler warning. (Usually you'd want : BaseClass(other); : BaseClass() is really just silencing the warning so you can handle BaseClass in the body of the copy constructor.)

That said, the warning appears to have been introduced in gcc 4.0 in 2005 so every modern version of gcc will produce this warning. So in practical terms the concerns raised by @mgehre are why I feel this option is needed.

poelmanc marked 2 inline comments as done.Oct 21 2019, 8:57 AM

Addressed these the other day but failed to check the "Done" boxes. Done!

poelmanc updated this revision to Diff 225897.Oct 21 2019, 8:59 AM

Rebase to latest master (tests moved into new "checkers" directory.)

What do @malcolm.parsons, @alexfh, @hokein, @aaron.ballman, @lebedev.ri think of @mgehre's suggestion to enable IgnoreBaseInCopyConstructors as the default setting, so gcc users won't experience build errors and think "clang-tidy broke my code!"

I could go either way and appreciate any input.

What do @malcolm.parsons, @alexfh, @hokein, @aaron.ballman, @lebedev.ri think of @mgehre's suggestion to enable IgnoreBaseInCopyConstructors as the default setting, so gcc users won't experience build errors and think "clang-tidy broke my code!"

I could go either way and appreciate any input.

I think GCC is being overly pedantic, but I don't see much harm in enabling the option by default.

clang-tools-extra/clang-tidy/readability/RedundantMemberInitCheck.cpp
60

You should elide the braces here per our coding standard.

poelmanc updated this revision to Diff 226847.Oct 29 2019, 12:29 AM

Changed default to 1, updated help accordingly, and removed an extraneous set of braces around a one-line statement.

poelmanc marked an inline comment as done.Oct 29 2019, 12:29 AM

I like that this check warns about copy constructors that don't copy.
The warning can be suppressed with a NOLINT comment if not copying is intentional.

I like that this check warns about copy constructors that don't copy.
The warning can be suppressed with a NOLINT comment if not copying is intentional.

Thanks for the comment! It's not clear to me how you suggest proceeding though.

I think it would be great to have a warning about "copy constructors that don't copy", which I would expect to warn regardless of whether the default initialization is explicit, e.g.

MyClass( const MyClass& other) : BaseClass() {...}`

or implicit, e.g.

MyClass( const MyClass& other) {...}

An intelligent "copy constructors that don't copy" warning might even examine the {...} to see what's being handled there.

Unfortunately currently we have two tools each of which warns in one case but not the other:

  • gcc -Wextra warns only in the latter case, and can be fixed by changing each occurrence to the former.
  • clang-tidy's current readability-redundant-member-init check warns only in the former case, and fixes it by changing each occurrence to the latter.
    • (Also clang-tidy's warning about : BaseClass() being redundant isn't a very clear warning about copy constructor that don't copy.)

This patch just offers an option to make peace between the two tools.

mgehre removed a subscriber: mgehre.Nov 9 2019, 12:26 AM

Exactly due to the issue you are fixing here, we ended up disabling the complete check because we didn't want to live with the warnings it produced on -Wextra.
Therefore, I'm actually strongly in favor to enable the option by default.

When others see that clang-tidy fixits introduce warnings (with -Wextra) or even break their build (with -Werror), they might not look into check options, but just disable the check directly.

Just pinging to see if anyone has any thoughts on moving forward with this. Thanks in advance for any feedback!

Exactly due to the issue you are fixing here, we ended up disabling the complete check because we didn't want to live with the warnings it produced on -Wextra.
Therefore, I'm actually strongly in favor to enable the option by default.

When others see that clang-tidy fixits introduce warnings (with -Wextra) or even break their build (with -Werror), they might not look into check options, but just disable the check directly.

Just pinging to see if anyone has any thoughts on moving forward with this. Thanks in advance for any feedback!

If this would default to off i'd signoff.

Exactly due to the issue you are fixing here, we ended up disabling the complete check because we didn't want to live with the warnings it produced on -Wextra.
Therefore, I'm actually strongly in favor to enable the option by default.

When others see that clang-tidy fixits introduce warnings (with -Wextra) or even break their build (with -Werror), they might not look into check options, but just disable the check directly.

Just pinging to see if anyone has any thoughts on moving forward with this. Thanks in advance for any feedback!

If this would default to off i'd signoff.

It sounds like you and @mgehre would like a different default for the new option? Personally, I don't have a strong opinion on the default, but I do think we should have the option so that users can control the behavior so it doesn't conflict between clang-tidy and gcc.

Exactly due to the issue you are fixing here, we ended up disabling the complete check because we didn't want to live with the warnings it produced on -Wextra.
Therefore, I'm actually strongly in favor to enable the option by default.

When others see that clang-tidy fixits introduce warnings (with -Wextra) or even break their build (with -Werror), they might not look into check options, but just disable the check directly.

Just pinging to see if anyone has any thoughts on moving forward with this. Thanks in advance for any feedback!

If this would default to off i'd signoff.

It sounds like you and @mgehre would like a different default for the new option?

I suppose. It's not unlike "clang -Wall is not supposed to match gcc's -Wall" issue.
Everyone may expect different behavior. Defaulting to on (don't warn) isn't misguided,
there's merit in not changing the code if it would break some other compiler the code supports.

But the other way around is also true, what if that gcc's diagnostic is not enabled,
or gcc is just not used? One then would need to actually know that there is an option
that should be flipped. Here i would prefer to have to disable something
if it's unwanted rather than somehow finding out that something can be enabled..

As a more meaningful data point, a similar relaxation to another check was added in D70165,
and there the default is to still warn - so not following the precedent seems inconsistent to me.

Personally, I don't have a strong opinion on the default, but

I do think we should have the option so that users can control the behavior so it doesn't conflict between clang-tidy and gcc.

Yes, i don't disagree that having *an ability* to control this is good.

poelmanc updated this revision to Diff 229934.Nov 18 2019, 3:16 PM
poelmanc edited the summary of this revision. (Show Details)

Switch default to 0. Add Release Note with some detail to increase the chances of someone finding this with an Internet search on the error message.

Thanks @lebedev.ri for taking the time to think this through and reply. All that makes sense, so I've changed the default to 0.

In addition to adding it to the ReleaseNotes, once clang-tidy 10 is released I'll reply to a StackOverflow question about this error with a link to the new option. That should further increase the odds of anyone who searches on the gcc warning/error easily finding the clang-tidy option.

lebedev.ri accepted this revision.Nov 18 2019, 11:15 PM

Thank you, looks good to me.

This revision is now accepted and ready to land.Nov 18 2019, 11:15 PM
This revision was automatically updated to reflect the committed changes.