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

[poelmanc]

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.

[mgehre]

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]

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

55

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.

[lebedev.ri]

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

[poelmanc]

In D69145#1715611, @mgehre wrote:

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]

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.

[poelmanc]

In D69145#1715614, @lebedev.ri wrote:

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]

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

[poelmanc]

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

[poelmanc]

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.

[aaron.ballman]

In D69145#1720254, @poelmanc wrote:

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

59

You should elide the braces here per our coding standard.

[poelmanc]

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

[malcolm.parsons]

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.

[poelmanc]

In D69145#1725026, @malcolm.parsons wrote:

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.

[poelmanc]

In D69145#1715611, @mgehre wrote:

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!

[lebedev.ri]

In D69145#1748716, @poelmanc wrote:

In D69145#1715611, @mgehre wrote:

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.

[aaron.ballman]

In D69145#1748733, @lebedev.ri wrote:

In D69145#1748716, @poelmanc wrote:

In D69145#1715611, @mgehre wrote:

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.

[lebedev.ri]

In D69145#1750529, @aaron.ballman wrote:

In D69145#1748733, @lebedev.ri wrote:

In D69145#1748716, @poelmanc wrote:

In D69145#1715611, @mgehre wrote:

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]

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.

[poelmanc]

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]

Thank you, looks good to me.

clang-tools-extra/clang-tidy/readability/RedundantMemberInitCheck.h

 /// Finds member initializations that are unnecessary because the same default
 /// constructor would be called if they were not present.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-member-init.html
 class RedundantMemberInitCheck : public ClangTidyCheck {
 public:
   RedundantMemberInitCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+      : ClangTidyCheck(Name, Context),
+        IgnoreBaseInCopyConstructors(Options.get("IgnoreBaseInCopyConstructors", 0))
+  {}
+  void storeOptions(ClangTidyOptions::OptionMap& Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+private:
+  bool IgnoreBaseInCopyConstructors;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_MEMBER_INIT_H

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

 
 using namespace clang::ast_matchers;
 using namespace clang::tidy::matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
+void RedundantMemberInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnoreBaseInCopyConstructors", IgnoreBaseInCopyConstructors );
+}
+
 void RedundantMemberInitCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
   auto Construct =
       cxxConstructExpr(
           hasDeclaration(cxxConstructorDecl(hasParent(
               cxxRecordDecl(unless(isTriviallyDefaultConstructible()))))))
           .bind("construct");
 
   Finder->addMatcher(
       cxxConstructorDecl(
           unless(isDelegatingConstructor()),
           ofClass(unless(
               anyOf(isUnion(), ast_matchers::isTemplateInstantiation()))),
           forEachConstructorInitializer(
               cxxCtorInitializer(isWritten(),
                                  withInitializer(ignoringImplicit(Construct)),
                                  unless(forField(hasType(isConstQualified()))),
                                  unless(forField(hasParent(recordDecl(isUnion())))))
-                  .bind("init"))),
+                  .bind("init")))
+          .bind("constructor"),
       this);
 }
 
 void RedundantMemberInitCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Init = Result.Nodes.getNodeAs<CXXCtorInitializer>("init");
   const auto *Construct = Result.Nodes.getNodeAs<CXXConstructExpr>("construct");
+  const auto* ConstructorDecl = Result.Nodes.getNodeAs<CXXConstructorDecl>( "constructor" );
+
+  if (IgnoreBaseInCopyConstructors && ConstructorDecl->isCopyConstructor() && Init->isBaseInitializer()) {
+    return;
+  }
 
   if (Construct->getNumArgs() == 0 ||
       Construct->getArg(0)->isDefaultArgument()) {
     if (Init->isAnyMemberInitializer()) {
       diag(Init->getSourceLocation(), "initializer for member %0 is redundant")
           << Init->getAnyMember()
           << FixItHint::CreateRemoval(Init->getSourceRange());
     } else {

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

 .. title:: clang-tidy - readability-redundant-member-init
 
 readability-redundant-member-init
 =================================
 
 Finds member initializations that are unnecessary because the same default
 constructor would be called if they were not present.
 
-Example:
+Example
+-------
 
 .. code-block:: c++
 
   // Explicitly initializing the member s is unnecessary.
   class Foo {
   public:
     Foo() : s() {}
 
   private:
     std::string s;
   };
+
+Options
+-------
+
+.. option:: IgnoreBaseInCopyConstructors
+
+    When non-zero, the check will ignore unnecessary base class initializations
+    within copy constructors. Some compilers issue warnings requiring copy
+    constructors to explicitly initialize their base classes.
+
+    Default is ``0``.  

clang-tools-extra/test/clang-tidy/readability-redundant-member-init.cpp

 // RUN: %check_clang_tidy %s readability-redundant-member-init %t
+// RUN:   -config="{CheckOptions: \
+// RUN:             [{key: readability-redundant-member-init.IgnoreBaseInCopyConstructors, \
+// RUN:               value: 1}] \
+// RUN:             }"
 
 struct S {
   S() = default;
   S(int i) : i(i) {}
   int i = 1;
 };
 
 struct T {
struct F9 {
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: initializer for member 's1' is redundant
   // CHECK-FIXES: F9()  {}
   struct {
     S s1;
     S s2;
   };
 };
 
+// struct whose inline copy constructor default-initializes its base class
+struct WithCopyConstructor1 : public T {
+  WithCopyConstructor1(const WithCopyConstructor1& other) : T(),
+    f(),
+    g()
+  {}
+  S f, g;
+};
+// No warning in copy constructor about T since IgnoreBaseInCopyConstructors=1
+// CHECK-MESSAGES: :[[@LINE-6]]:5: warning: initializer for member 'f' is redundant
+// CHECK-MESSAGES: :[[@LINE-6]]:5: warning: initializer for member 'g' is redundant
+// CHECK-FIXES: WithCopyConstructor1(const WithCopyConstructor1& other) : T()
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: {}
+
+// struct whose copy constructor default-initializes its base class
+struct WithCopyConstructor2 : public T {
+  WithCopyConstructor2(const WithCopyConstructor2& other);
+  S a;
+};
+WithCopyConstructor2::WithCopyConstructor2(const WithCopyConstructor2& other)
+  : T(), a()
+{}
+// No warning in copy constructor about T since IgnoreBaseInCopyConstructors=1
+// CHECK-MESSAGES: :[[@LINE-3]]:10: warning: initializer for member 'a' is redundant
+// CHECK-FIXES: {{^}}  : T() {{$}}
+// CHECK-NEXT: {}
+
 // Initializer not written
 struct NF1 {
   NF1() {}
   S f;
 };
 
 // Initializer doesn't call default constructor
 struct NF2 {