[clang-tidy]: modernize-use-default-member-init: Remove trailing comma and colon.

[jdemeule]

Summary

'modernize-user-default-member-init' does not automatically ask to remove comma and colon when replacements are produced.

It seems, when they are apply directly from clang-tidy, the RefactoringTool engine is smart enough to remove trailing tokens.
However, when fixes are exported, clang-apply-replacements cannot do the same trick and will lead trailing commas and colon as reported on https://bugs.llvm.org/show_bug.cgi?id=35051.

This patch made “modernize-use-default-member-init” generate explicitly the removal of these locations to made it work correctly in both case (-fix and -export-fixes).

[Eugene.Zelenko]

clang-tidy/modernize/UseDefaultMemberInitCheck.cpp

15

Please place it on a top, since header belong to project.

26

Please use while (true) instead.

48

Unnecessary curvy brackets.

177

Unnecessary initializer. See readability-redundant-member-init.

271

Unnecessary curvy brackets.

298

Unnecessary curvy brackets.

302

Unnecessary curvy brackets.

308

Unnecessary curvy brackets.

317

Unnecessary curvy brackets.

unittests/clang-tidy/ModernizerModuleTest.cpp

62

Please separate function from namespaces ends with empty line.

[aaron.ballman]

It seems, when they are apply directly from clang-tidy, the RefactoringTool engine is smart enough to remove trailing tokens. However, when fixes are exported, clang-apply-replacements cannot do the same trick and will lead trailing commas and colon

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

clang-tidy/modernize/UseDefaultMemberInitCheck.cpp

26

Better still, don't use an infinite loop.

[malcolm.parsons]

In D43500#1013497, @aaron.ballman wrote:

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

See http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161017/174238.html

[jdemeule]

In D43500#1013558, @malcolm.parsons wrote:

In D43500#1013497, @aaron.ballman wrote:

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

See http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161017/174238.html

I was not aware of cleanupAroundReplacements. It should be a better option than fixing every check one by one. I am working on adding it on clang-apply-replacement for now and another review will be propose soon.

Should we discard this patch or keep only the added tests (if you found them relevant after fixing the comments)?

[aaron.ballman]

In D43500#1015208, @jdemeule wrote:

In D43500#1013558, @malcolm.parsons wrote:

In D43500#1013497, @aaron.ballman wrote:

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

See http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161017/174238.html

I was not aware of cleanupAroundReplacements. It should be a better option than fixing every check one by one. I am working on adding it on clang-apply-replacement for now and another review will be propose soon.

Should we discard this patch or keep only the added tests (if you found them relevant after fixing the comments)?

I'd keep the test cases, as they're good examples for us to ensure we don't regress.

[alexfh]

In D43500#1013497, @aaron.ballman wrote:

It seems, when they are apply directly from clang-tidy, the RefactoringTool engine is smart enough to remove trailing tokens. However, when fixes are exported, clang-apply-replacements cannot do the same trick and will lead trailing commas and colon

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

Eric was going to look at this, IIRC.

[ioeric]

In D43500#1015208, @jdemeule wrote:

In D43500#1013558, @malcolm.parsons wrote:

In D43500#1013497, @aaron.ballman wrote:

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

See http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161017/174238.html

I was not aware of cleanupAroundReplacements. It should be a better option than fixing every check one by one. I am working on adding it on clang-apply-replacement for now and another review will be propose soon.

That would be awesome Jeremy! Thanks!

I think it might be easier if you convert all replacements to tooling::AtomicChange and use applyAtomicChanges (https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/AtomicChange.h#L172) in clang-apply-replacements.

Let me know if you have any question, and I'm happy to review the patch when it's ready!

[jdemeule]

As discuss, I have removed the modifications but keep the tests.
To have tests with meaning, I have also add a way to format code on 'runCheckOnCode'.

[aaron.ballman]

unittests/clang-tidy/ClangTidyTest.h

145 ↗

(On Diff #135895)

Extraneous whitespace.

147 ↗

(On Diff #135895)

Please don't use auto unless the type is explicitly spelled out in the initialization. (Here and elsewhere in the patch.)

159–160 ↗

(On Diff #135895)

This smells like it should be an assert rather than an unreachable.

167 ↗

(On Diff #135895)

Are you intending to implement this fixme in this patch?

[jdemeule]

unittests/clang-tidy/ClangTidyTest.h

159–160 ↗

(On Diff #135895)

Is returning an empty string OK?
This should let the assertion on the test do the job.

167 ↗

(On Diff #135895)

I suggest to remove the copy/paste containing the fixme. Is-it OK?
Maybe with some hints I can suggest a fix for this later.

[aaron.ballman]

unittests/clang-tidy/ClangTidyTest.h

159–160 ↗

(On Diff #135895)

Is returning an empty string OK?

It seems like a reasonable behavior, but I've not tried it myself.

This should let the assertion on the test do the job.

If an assertion fails with assertions disabled, the resulting UB is generally acceptable. Because these are unit tests, I think we can generally rely on the assertion being enabled if this code is being run.

167 ↗

(On Diff #135895)

Ah, I didn't see that this was a copy paste from below. I think those code paths could be combined if you hoisted the code out to the function level.

[jdemeule]

Update after review.

[alexfh]

unittests/clang-tidy/ClangTidyTest.h

145 ↗

(On Diff #136389)

I wonder whether it's better to use lit for the tests that require formatting than to expand clang-tidy unit test utilities with runCheckAndFormatOnCode? What was the reason to use unit tests in this patch as opposed to lit tests?

[jdemeule]

unittests/clang-tidy/ClangTidyTest.h

145 ↗

(On Diff #136389)

Indeed, that a good question.
Personally, I found unit test easier to launch and debug than lit test and I notice that it is not possible to made a unit test which react the same way as clang-tidy.
When I wrote this test, I want it to react as in clang-tidy and for this I try to call cleanupAroundReplacements in the test.
Adding formatting was more a side effect to have some sustainable and comprehensive way to write test.

[alexfh]

unittests/clang-tidy/ClangTidyTest.h

145 ↗

(On Diff #136389)

With more features clang-tidy gets the discrepancies between how unit tests work and how clang-tidy works become larger. I wouldn't like to mirror every clang-tidy feature in the code that supports unit tests. We should either expose more high-level APIs or use clang-tidy itself and lit tests. (I personally like the latter more, since lit tests tend to be closer to the real use cases and are easier to write and iterate on.)

[JonasToth]

What is the status here? Will you continue to work on this patch @jdemeule (which would be great!) ?

[chgans]

Hi, Using llvm-8 (llvm ubuntu repo), clang-replacement has improved, but there still a case when a comma is left beyond: if there was only one member init.

[jdemeule]

In D43500#1244518, @JonasToth wrote:

What is the status here? Will you continue to work on this patch @jdemeule (which would be great!) ?

Hi, now, this patch contains only tests which are already covered with lit tests.
The cleanup is now applied in clang-apply-replacements.

In D43500#1491369, @chgans wrote:

Hi, Using llvm-8 (llvm ubuntu repo), clang-replacement has improved, but there still a case when a comma is left beyond: if there was only one member init.

I do not reproduce at master (9.0.0 trunk), do you have an example in mind?

[lebedev.ri]

In D43500#1546077, @jdemeule wrote:

In D43500#1244518, @JonasToth wrote:

What is the status here? Will you continue to work on this patch @jdemeule (which would be great!) ?

Hi, now, this patch contains only tests which are already covered with lit tests.
The cleanup is now applied in clang-apply-replacements.

How does that work with clang-tidy -fix ?

In D43500#1491369, @chgans wrote:

Hi, Using llvm-8 (llvm ubuntu repo), clang-replacement has improved, but there still a case when a comma is left beyond: if there was only one member init.

I do not reproduce at master (9.0.0 trunk), do you have an example in mind?

[jdemeule]

In D43500#1546089, @lebedev.ri wrote:

In D43500#1546077, @jdemeule wrote:

In D43500#1244518, @JonasToth wrote:

What is the status here? Will you continue to work on this patch @jdemeule (which would be great!) ?

Hi, now, this patch contains only tests which are already covered with lit tests.
The cleanup is now applied in clang-apply-replacements.

How does that work with clang-tidy -fix ?

If I remember by calling 'format::cleanupAroundReplacements' when applying the fix in memory.

clang-tidy/modernize/UseDefaultMemberInitCheck.h

 public:
   UseDefaultMemberInitCheck(StringRef Name, ClangTidyContext *Context);
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   void checkDefaultInit(const ast_matchers::MatchFinder::MatchResult &Result,
-                        const CXXCtorInitializer *Init);
+                        const CXXCtorInitializer *Init,
+                        const CXXConstructorDecl *Ctor);
   void checkExistingInit(const ast_matchers::MatchFinder::MatchResult &Result,
-                         const CXXCtorInitializer *Init);
+                         const CXXCtorInitializer *Init,
+                         const CXXConstructorDecl *Ctor);
+
+  void removeTrailingColon(DiagnosticBuilder &Diag, const ASTContext *Context,
+                           const CXXConstructorDecl *Ctor);
 
   const bool UseAssignment;
   const bool IgnoreMacros;
+  std::map<const CXXConstructorDecl *, unsigned int> RemovedInitializers;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_DEFAULT_MEMBER_INIT_H

clang-tidy/modernize/UseDefaultMemberInitCheck.cpp

 //===--- UseDefaultMemberInitCheck.cpp - clang-tidy------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "UseDefaultMemberInitCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 
+#include "../utils/LexerUtils.h"
+
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
+static SourceLocation findPreviousLocation(const ASTContext *Context,
+                                           SourceLocation Location,
+                                           tok::TokenKind K) {
+  for (;;) {
+    Token Tok = utils::lexer::getPreviousToken(*Context, Location);
+    SourceLocation TokLoc = Tok.getLocation();
+    if (TokLoc.isInvalid() || Tok.is(K))
+      return TokLoc;
+  }
+}
+
+static SourceLocation findColonLocation(const ASTContext *Context,
+                                        SourceLocation Location) {
+  return findPreviousLocation(Context, Location, tok::colon);
+}
+
+static SourceLocation findPreviousCommaLocation(const ASTContext *Context,
+                                                SourceLocation Location) {
+  return findPreviousLocation(Context, Location, tok::comma);
+}
+
+static SourceLocation findInitEndLoc(const CXXCtorInitializer *Init,
+                                     const CXXConstructorDecl *Ctor) {
+  auto pos = std::find(Ctor->init_begin(), Ctor->init_end(), Init);
+  auto NextInit = std::next(pos);
+  if (NextInit != Ctor->init_end()) {
+    return (*NextInit)->getSourceLocation();
+  }
+  return Ctor->getBody()->getLocStart();
+}
+
 static StringRef getValueOfValueInit(const QualType InitType) {
   switch (InitType->getScalarTypeKind()) {
   case Type::STK_CPointer:
   case Type::STK_BlockPointer:
   case Type::STK_ObjCObjectPointer:
   case Type::STK_MemberPointer:
     return "nullptr";
 
default:
     return false;
   }
 }
 
 UseDefaultMemberInitCheck::UseDefaultMemberInitCheck(StringRef Name,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       UseAssignment(Options.get("UseAssignment", 0) != 0),
-      IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true) != 0) {}
+      IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true) != 0),
+      RemovedInitializers() {}
 
 void UseDefaultMemberInitCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "UseAssignment", UseAssignment);
   Options.store(Opts, "IgnoreMacros", IgnoreMacros);
 }
 
 void UseDefaultMemberInitCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
           forEachConstructorInitializer(
               cxxCtorInitializer(
                   forField(unless(anyOf(getLangOpts().CPlusPlus2a
                                             ? unless(anything())
                                             : isBitField(),
                                         hasInClassInitializer(anything()),
                                         hasParent(recordDecl(isUnion()))))),
                   isWritten(), withInitializer(ignoringImplicit(Init)))
-                  .bind("default"))),
+                  .bind("default")))
+          .bind("ctor"),
       this);
 
   Finder->addMatcher(
       cxxConstructorDecl(
           unless(ast_matchers::isTemplateInstantiation()),
           forEachConstructorInitializer(
               cxxCtorInitializer(forField(hasInClassInitializer(anything())),
                                  isWritten(),
                                  withInitializer(ignoringImplicit(Init)))
-                  .bind("existing"))),
+                  .bind("existing")))
+          .bind("ctor"),
       this);
 }
 
 void UseDefaultMemberInitCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
   if (const auto *Default =
           Result.Nodes.getNodeAs<CXXCtorInitializer>("default"))
-    checkDefaultInit(Result, Default);
+    checkDefaultInit(Result, Default, Ctor);
   else if (const auto *Existing =
                Result.Nodes.getNodeAs<CXXCtorInitializer>("existing"))
-    checkExistingInit(Result, Existing);
+    checkExistingInit(Result, Existing, Ctor);
   else
     llvm_unreachable("Bad Callback. No node provided.");
 }
 
 void UseDefaultMemberInitCheck::checkDefaultInit(
-    const MatchFinder::MatchResult &Result, const CXXCtorInitializer *Init) {
+    const MatchFinder::MatchResult &Result, const CXXCtorInitializer *Init,
+    const CXXConstructorDecl *Ctor) {
   const FieldDecl *Field = Init->getAnyMember();
 
   SourceLocation StartLoc = Field->getLocStart();
   if (StartLoc.isMacroID() && IgnoreMacros)
     return;
 
   SourceLocation FieldEnd =
       Lexer::getLocForEndOfToken(Field->getSourceRange().getEnd(), 0,
void UseDefaultMemberInitCheck::checkDefaultInit(
 
   if (UseAssignment && isa<ImplicitValueInitExpr>(Init->getInit()))
     Diag << FixItHint::CreateInsertion(
         FieldEnd, getValueOfValueInit(Init->getInit()->getType()));
 
   if (!UseAssignment)
     Diag << FixItHint::CreateInsertion(FieldEnd, "}");
 
-  Diag << FixItHint::CreateRemoval(Init->getSourceRange());
+  Diag << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+      Init->getSourceLocation(), findInitEndLoc(Init, Ctor)));
+
+  if (++RemovedInitializers[Ctor] == Ctor->getNumCtorInitializers()) {
+    removeTrailingColon(Diag, Result.Context, Ctor);
+  }
 }
 
 void UseDefaultMemberInitCheck::checkExistingInit(
-    const MatchFinder::MatchResult &Result, const CXXCtorInitializer *Init) {
+    const MatchFinder::MatchResult &Result, const CXXCtorInitializer *Init,
+    const CXXConstructorDecl *Ctor) {
   const FieldDecl *Field = Init->getMember();
 
   if (!sameValue(Field->getInClassInitializer(), Init->getInit()))
     return;
 
+  auto Diag =
       diag(Init->getSourceLocation(), "member initializer for %0 is redundant")
-      << Field
-      << FixItHint::CreateRemoval(Init->getSourceRange());
+      << Field;
+
+  auto pos = std::find(Ctor->init_begin(), Ctor->init_end(), Init) -
+             Ctor->init_begin();
+  if (Ctor->getNumCtorInitializers() == 1) {
+    // Only 1 initializer, removing the SourceRange is fine, the colon will be
+    // removed later.
+    Diag << FixItHint::CreateRemoval(Init->getSourceRange());
+  } else if (pos == 0) {
+    // We are removing the 1st initializer and are avoiding to keep ': ,'.
+    const auto *NextInit = *std::next(Ctor->init_begin());
+    if (sameValue(NextInit->getMember()->getInClassInitializer(),
+                  NextInit->getInit())) {
+      // The next initializer will be removed later. Removing only the
+      // initializer is enough. Colon will be treated later.
+      Diag << FixItHint::CreateRemoval(Init->getSourceRange());
+    } else {
+      // The next initializer will not be remove. In this case, we should create
+      // a removal to the next ',' or the Ctor body.
+      Diag << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+          Init->getSourceLocation(), findInitEndLoc(Init, Ctor)));
+    }
+  } else {
+    // This initializer is in the middle of other one.
+    // In this case, we can create a removal from the previous ',' to the end of
+    // the initializer.
+    Diag << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+        findPreviousCommaLocation(Result.Context, Init->getSourceLocation()),
+        Init->getRParenLoc().getLocWithOffset(1)));
+  }
+
+  if (++RemovedInitializers[Ctor] == Ctor->getNumCtorInitializers()) {
+    removeTrailingColon(Diag, Result.Context, Ctor);
+  }
+}
+
+void UseDefaultMemberInitCheck::removeTrailingColon(
+    DiagnosticBuilder &Diag, const ASTContext *Context,
+    const CXXConstructorDecl *Ctor) {
+  const CXXCtorInitializer *FirstInit = *Ctor->init_begin();
+  SourceLocation ColonLoc =
+      findColonLocation(Context, FirstInit->getSourceLocation());
+
+  if (ColonLoc.isInvalid() || ColonLoc.isMacroID())
+    return;
+
+  Diag << FixItHint::CreateRemoval(ColonLoc);
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

unittests/clang-tidy/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   )
 
 get_filename_component(CLANG_LINT_SOURCE_DIR
   ${CMAKE_CURRENT_SOURCE_DIR}/../../clang-tidy REALPATH)
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
   GoogleModuleTest.cpp
   LLVMModuleTest.cpp
+  ModernizerModuleTest.cpp
   NamespaceAliaserTest.cpp
   ObjCModuleTest.cpp
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp)
 
 target_link_libraries(ClangTidyTests
   PRIVATE
   clangAST
   clangASTMatchers
   clangBasic
   clangFrontend
   clangLex
   clangTidy
   clangTidyAndroidModule
   clangTidyGoogleModule
   clangTidyLLVMModule
+  clangTidyModernizeModule
   clangTidyObjCModule
   clangTidyReadabilityModule
   clangTidyUtils
   clangTooling
   clangToolingCore
   )

unittests/clang-tidy/ModernizerModuleTest.cpp

+#include "ClangTidyTest.h"
+#include "modernize/UseDefaultMemberInitCheck.h"
+#include "gtest/gtest.h"
+
+using namespace clang::tidy::modernize;
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+TEST(UseDefaultMemberInitCheckTest, NoChanges) {
+  EXPECT_NO_CHANGES(
+      UseDefaultMemberInitCheck,
+      "struct S { S() = default; bool a_ = false; bool b_ = false; };");
+  EXPECT_NO_CHANGES(UseDefaultMemberInitCheck, "struct S { S() : a_(true), "
+                                               "b_(true) {} bool a_{false}; "
+                                               "bool b_{false}; };");
+}
+
+TEST(UseDefaultMemberInitCheckTest, Basic) {
+  EXPECT_EQ("struct S { S() {} bool a_{false}; };",
+            runCheckOnCode<UseDefaultMemberInitCheck>(
+                "struct S { S() : a_(false) {} bool a_; };"));
+}
+
+TEST(UseDefaultMemberInitCheckTest, SeveralInitializers) {
+  EXPECT_EQ(
+      "struct S { S() {} bool a_{false}; bool b_{true}; };",
+      runCheckOnCode<UseDefaultMemberInitCheck>(
+          "struct S { S() : a_(false), b_(true) {} bool a_; bool b_; };"));
+}
+
+TEST(UseDefaultMemberInitCheckTest, ExceptSpec) {
+  EXPECT_EQ(
+      "struct S { S() noexcept(true) {} bool a_{false}; bool b_{true}; };",
+      runCheckOnCode<UseDefaultMemberInitCheck>(
+          "struct S { S() noexcept(true) : a_(false), b_(true) {} bool a_; "
+          "bool b_; };"));
+  EXPECT_EQ(
+      "#define NOEXCEPT_(X) noexcept(X)\n"
+      "struct S { S() NOEXCEPT_(true) {} bool a_{false}; bool b_{true}; };",
+      runCheckOnCode<UseDefaultMemberInitCheck>(
+          "#define NOEXCEPT_(X) noexcept(X)\n"
+          "struct S { S() NOEXCEPT_(true) : a_(false), b_(true) {} bool a_; "
+          "bool b_; };"));
+}
+
+TEST(UseDefaultMemberInitCheckTest, OnExisting) {
+  EXPECT_EQ("struct S { S()  {} bool a_{false}; bool b_{true}; };",
+            runCheckOnCode<UseDefaultMemberInitCheck>(
+                "struct S { S() : a_(false), b_(true) {} bool a_{false}; bool "
+                "b_{true}; };"));
+  EXPECT_EQ("struct S { S() : a_(true) {} bool a_{false}; bool b_{true}; };",
+            runCheckOnCode<UseDefaultMemberInitCheck>(
+                "struct S { S() : a_(true), b_(true) {} bool a_{false}; bool "
+                "b_{true}; };"));
+  EXPECT_EQ("struct S { S() : b_(false) {} bool a_{false}; bool b_{true}; };",
+            runCheckOnCode<UseDefaultMemberInitCheck>(
+                "struct S { S() : a_(false), b_(false) {} bool a_{false}; bool "
+                "b_{true}; };"));
+}
+} // namespace test
+} // namespace tidy
+} // namespace clang