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

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.

Reviewers: malcolm.parsons, alexfh, hokein, aaron.ballman, lebedev.ri

Patch by: poelmanc

Subscribers: mgehre, lebedev.ri, cfe-commits

Tags: #clang, #clang-tools-extra

Differential revision: https://reviews.llvm.org/D69145

Author: Mitchell Balan

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

@@ -20,6 +20,11 @@ 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;
@@ -36,17 +41,24 @@ void RedundantMemberInitCheck::registerMatchers(MatchFinder *Finder) {
           ofClass(unless(
               anyOf(isUnion(), ast_matchers::isTemplateInstantiation()))),
           forEachConstructorInitializer(
-              cxxCtorInitializer(isWritten(),
-                                 withInitializer(ignoringImplicit(Construct)),
-                                 unless(forField(hasType(isConstQualified()))),
-                                 unless(forField(hasParent(recordDecl(isUnion())))))
-                  .bind("init"))),
+              cxxCtorInitializer(
+                  isWritten(), withInitializer(ignoringImplicit(Construct)),
+                  unless(forField(hasType(isConstQualified()))),
+                  unless(forField(hasParent(recordDecl(isUnion())))))
+                  .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()) {

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

@@ -23,9 +23,15 @@ namespace readability {
 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

clang-tools-extra/docs/ReleaseNotes.rst

@@ -176,6 +176,13 @@ Improvements to clang-tidy
   The check now supports the ``AllowOverrideAndFinal`` option to eliminate
   conflicts with ``gcc -Wsuggest-override`` or ``gcc -Werror=suggest-override``.
 
+- Improved :doc:`readability-redundant-member-init
+  <clang-tidy/checks/readability-redundant-member-init>` check.
+
+  The check  now supports the ``IgnoreBaseInCopyConstructors`` option to avoid
+  `"base class \‘Foo\’ should be explicitly initialized in the copy constructor"`
+  warnings or errors with ``gcc -Wextra`` or ``gcc -Werror=extra``.
+
 - The :doc:`readability-redundant-string-init
   <clang-tidy/checks/readability-redundant-string-init>` check now supports a
   `StringNames` option enabling its application to custom string classes.

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

@@ -6,7 +6,8 @@ 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++
 
@@ -18,3 +19,27 @@ Example:
   private:
     std::string s;
   };
+
+Options
+-------
+
+.. option:: IgnoreBaseInCopyConstructors
+
+    Default is ``0``.
+
+    When non-zero, the check will ignore unnecessary base class initializations
+    within copy constructors, since some compilers issue warnings/errors when
+    base classes are not explicitly intialized in copy constructors. For example,
+    ``gcc`` with ``-Wextra`` or ``-Werror=extra`` issues warning or error
+    ``base class ‘Bar’ should be explicitly initialized in the copy constructor``
+    if ``Bar()`` were removed in the following example:
+
+.. code-block:: c++
+
+  // Explicitly initializing member s and base class Bar is unnecessary.
+  struct Foo : public Bar {
+    // Remove s() below. If IgnoreBaseInCopyConstructors!=0, keep Bar().
+    Foo(const Foo& foo) : Bar(), s() {}
+    std::string s;
+  };
+

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

@@ -1,4 +1,8 @@
-// RUN: %check_clang_tidy %s readability-redundant-member-init %t
+// 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;
@@ -116,6 +120,35 @@ struct F9 {
   };
 };
 
+// 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() {}