clang-tidy: improve the 'modernize-use-default-member-init'

[oleg.smolsky]

Summary

• we want to deal with non-default constructors that just happen to contain constant initializers

There was already a negative test case, it is now a positive one. We find and refactor this case:

struct PositiveNotDefaultInt {
  PositiveNotDefaultInt(int) : i(7) {}
  int i;
};

[oleg.smolsky]

Fixing the uploaded diff...

[oleg.smolsky]

Ran clang-format

[oleg.smolsky]

Ran clang-format on the test cases.

[oleg.smolsky]

@aaron.ballman do you happen to know who is best to review a clang-tidy fix?

[aaron.ballman]

Adding some additional reviewers. At a high level, I think this is a reasonable direction to go, but I wonder if this should be hidden behind a configuration option or not (basically, was there a reason we didn't cover that case originally or was it an oversight/left for future work)?

clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp

48–49

The "default" here is not about the constructor but about the argument to the constructor, I believe. So I think this should be renamed to PositiveConstantMemInit or something like that (similar for the names below).

[oleg.smolsky]

Sure, adding an option is easy, if that's the consensus. What would you call it?

[malcolm.parsons]

In D114995#3180475, @aaron.ballman wrote:

was there a reason we didn't cover that case originally or was it an oversight/left for future work?

It was left for future work - by only considering the initializer list of the default constructor, clang-tidy did not have to work out what to do when the constructors don't agree on what value the member init should have.
The next step towards the full solution is to handle classes with only one non-special constructor.

[aaron.ballman]

In D114995#3183240, @malcolm.parsons wrote:

In D114995#3180475, @aaron.ballman wrote:

was there a reason we didn't cover that case originally or was it an oversight/left for future work?

It was left for future work - by only considering the initializer list of the default constructor, clang-tidy did not have to work out what to do when the constructors don't agree on what value the member init should have.

Thank you for verifying! @oleg.smolsky -- this would be a very useful test case to add, btw.

In D114995#3181486, @oleg.smolsky wrote:

Sure, adding an option is easy, if that's the consensus. What would you call it?

Since we left this for future work, I don't think we need to add a configuration option unless a user finds a need for one.

[oleg.smolsky]

Added a "two constructors" test case along with the support for that.

[oleg.smolsky]

In D114995#3198033, @aaron.ballman wrote:

In D114995#3183240, @malcolm.parsons wrote:

In D114995#3180475, @aaron.ballman wrote:

was there a reason we didn't cover that case originally or was it an oversight/left for future work?

It was left for future work - by only considering the initializer list of the default constructor, clang-tidy did not have to work out what to do when the constructors don't agree on what value the member init should have.

Thank you for verifying! @oleg.smolsky -- this would be a very useful test case to add, btw.

Yep, done.

PLAL

[aaron.ballman]

Mostly just minor nits at this point. Please be sure to update the documentation and add a release note about the changes.

clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp

6

Spurious newline?

250

251–252

[oleg.smolsky]

Addressed review comments: explicit types and doc changes.

[oleg.smolsky]

Done with review changes.

clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp

6

yep, removing it...

[aaron.ballman]

LGTM

[oleg.smolsky]

@aaron.ballman could you commit this change please? I've never had commit rights... Thanks!

[aaron.ballman]

In D114995#3218004, @oleg.smolsky wrote:

@aaron.ballman could you commit this change please? I've never had commit rights... Thanks!

Sure can! Is Oleg Smolsky <[e-mail]> the correct attribution for the patch, or would you like me to use a different name or email address?

[oleg.smolsky]

In D114995#3218022, @aaron.ballman wrote:

In D114995#3218004, @oleg.smolsky wrote:

@aaron.ballman could you commit this change please? I've never had commit rights... Thanks!

Sure can! Is Oleg Smolsky <[e-mail]> the correct attribution for the patch, or would you like me to use a different name or email address?

Thanks Aaron! Yes, everything here looks right.

[aaron.ballman]

Thanks for the improvement! I've commit on your behalf in 051847cfecaea3f55fc4f822facfbf5d21bde8dd.

clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp

 //===--- UseDefaultMemberInitCheck.cpp - clang-tidy------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "UseDefaultMemberInitCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
auto InitBase =
             declRefExpr(to(enumConstantDecl())));
 
   auto Init =
       anyOf(initListExpr(anyOf(allOf(initCountIs(1), hasInit(0, InitBase)),
                                initCountIs(0))),
             InitBase);
 
   Finder->addMatcher(
-      cxxConstructorDecl(
-          isDefaultConstructor(),
-          forEachConstructorInitializer(
-              cxxCtorInitializer(
-                  forField(unless(anyOf(getLangOpts().CPlusPlus20
-                                            ? unless(anything())
-                                            : isBitField(),
-                                        hasInClassInitializer(anything()),
-                                        hasParent(recordDecl(isUnion()))))),
-                  withInitializer(Init))
-                  .bind("default"))),
+      cxxConstructorDecl(forEachConstructorInitializer(
+              forField(unless(anyOf(
+                  getLangOpts().CPlusPlus20 ? unless(anything()) : isBitField(),
       this);
 
   Finder->addMatcher(
       cxxConstructorDecl(forEachConstructorInitializer(
           cxxCtorInitializer(forField(hasInClassInitializer(anything())),
                              withInitializer(Init))
               .bind("existing"))),
       this);
void UseDefaultMemberInitCheck::check(const MatchFinder::MatchResult &Result) {
   else
     llvm_unreachable("Bad Callback. No node provided.");
 }
 
 void UseDefaultMemberInitCheck::checkDefaultInit(
     const MatchFinder::MatchResult &Result, const CXXCtorInitializer *Init) {
   const FieldDecl *Field = Init->getAnyMember();
 
+  // Check whether we have multiple hand-written constructors and bomb out, as
+  // it is hard to reconcile their sets of member initializers.
+  const auto *ClassDecl = dyn_cast<CXXRecordDecl>(Field->getParent());
+  if (llvm::count_if(ClassDecl->ctors(), [](const CXXConstructorDecl *Ctor) {
+        return !Ctor->isCopyOrMoveConstructor();
+      }) > 1)
+    return;
+
   SourceLocation StartLoc = Field->getBeginLoc();
   if (StartLoc.isMacroID() && IgnoreMacros)
     return;
 
   SourceLocation FieldEnd =
       Lexer::getLocForEndOfToken(Field->getSourceRange().getEnd(), 0,
                                  *Result.SourceManager, getLangOpts());
   SourceLocation LParenEnd = Lexer::getLocForEndOfToken(

clang-tools-extra/docs/ReleaseNotes.rst

 --------------------------
 
 - Added support for globbing in `NOLINT*` expressions, to simplify suppressing
   multiple warnings in the same line.
 
 - Added support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress
   Clang-Tidy warnings over multiple lines.
 
+- Generalized the `modernize-use-default-member-init` check to handle non-default
+  constructors.
+
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-stringview-nullptr
   <clang-tidy/checks/bugprone-stringview-nullptr>` check.
 
   Checks for various ways that the ``const CharT*`` constructor of
   ``std::basic_string_view`` can be passed a null argument.

clang-tools-extra/docs/clang-tidy/checks/modernize-use-default-member-init.rst

 .. title:: clang-tidy - modernize-use-default-member-init
 
 modernize-use-default-member-init
 =================================
 
-This check converts a default constructor's member initializers into the new
+This check converts constructors' member initializers into the new
 default member initializers in C++11. Other member initializers that match the
 default member initializer are removed. This can reduce repeated code or allow
 use of '= default'.
 
 .. code-block:: c++
 
   struct A {
     A() : i(5), j(10.0) {}

clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp

 struct PositiveInt {
   PositiveInt() : j(1) {}
   // CHECK-FIXES: PositiveInt()  {}
   int j;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'j'
   // CHECK-FIXES: int j{1};
 };
 
+struct PositiveNotDefaultInt {
+  PositiveNotDefaultInt(int) : i(7) {}
+  // CHECK-FIXES: PositiveNotDefaultInt(int)  {}
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+// We cannot reconcile these initializers.
+struct TwoConstructors {
+    TwoConstructors(int) : i(7) {}
+    TwoConstructors(int, int) : i(8) {}
+    int i;
+};
+
+struct PositiveNotDefaultOOLInt {
+  PositiveNotDefaultOOLInt(int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int) : i(7) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int)  {}
+
+struct PositiveNotDefaultOOLInt2 {
+  PositiveNotDefaultOOLInt2(int, int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+  int j;
+};
+
+PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) : i(7), j(arg) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) :  j(arg) {}
+
 struct PositiveUnaryMinusInt {
   PositiveUnaryMinusInt() : j(-1) {}
   // CHECK-FIXES: PositiveUnaryMinusInt()  {}
   int j;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'j'
   // CHECK-FIXES: int j{-1};
 };
 

 };
 
 struct NegativeBitField
 {
   NegativeBitField() : i(6) {}
   int i : 5;
 };
 
-struct NegativeNotDefaultInt
-{
-  NegativeNotDefaultInt(int) : i(7) {}
-  int i;
-};
-
 struct NegativeDefaultArg
 {
   NegativeDefaultArg(int i = 4) : i(i) {}
   int i;
 };
 
 struct ExistingChar {
   ExistingChar(short) : e1(), e2{}, e3(), e4() {}