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

-2

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

-2

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
 //
 //===----------------------------------------------------------------------===//
 
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::checkDefaultInit(
     const MatchFinder::MatchResult &Result, const CXXCtorInitializer *Init) {
   const FieldDecl *Field = Init->getAnyMember();
 
   SourceLocation StartLoc = Field->getBeginLoc();
   if (StartLoc.isMacroID() && IgnoreMacros)
     return;
 
   SourceLocation FieldEnd =
       Lexer::getLocForEndOfToken(Field->getSourceRange().getEnd(), 0,
                                  *Result.SourceManager, getLangOpts());
   SourceLocation LParenEnd = Lexer::getLocForEndOfToken(
       Init->getLParenLoc(), 0, *Result.SourceManager, getLangOpts());
   CharSourceRange InitRange =
       CharSourceRange::getCharRange(LParenEnd, Init->getRParenLoc());
 
   bool ValueInit = isa<ImplicitValueInitExpr>(Init->getInit());

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};
+};
+
+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() {}