[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 ↗

(On Diff #135012)

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

26 ↗

(On Diff #135012)

Please use while (true) instead.

48 ↗

(On Diff #135012)

Unnecessary curvy brackets.

177 ↗

(On Diff #135012)

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

271 ↗

(On Diff #135012)

Unnecessary curvy brackets.

298 ↗

(On Diff #135012)

Unnecessary curvy brackets.

302 ↗

(On Diff #135012)

Unnecessary curvy brackets.

308 ↗

(On Diff #135012)

Unnecessary curvy brackets.

317 ↗

(On Diff #135012)

Unnecessary curvy brackets.

unittests/clang-tidy/ModernizerModuleTest.cpp

63

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 ↗

(On Diff #135012)

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

Extraneous whitespace.

147

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

159–160

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

167

Are you intending to implement this fixme in this patch?

[jdemeule]

unittests/clang-tidy/ClangTidyTest.h

159–160

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

167

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

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

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

144

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

144

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

144

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.

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/ClangTidyTest.h

 //===--- ClangTidyTest.h - clang-tidy ---------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANG_TIDY_CLANGTIDYTEST_H
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANG_TIDY_CLANGTIDYTEST_H
 
 #include "ClangTidy.h"
 #include "ClangTidyDiagnosticConsumer.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Format/Format.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/Path.h"
 #include <map>
 #include <memory>
for (const auto &FileAndFixes : Error.Fix) {
           llvm::errs() << llvm::toString(std::move(Err)) << "\n";
           return "";
         }
       }
     }
   }
   if (Errors)
     *Errors = Context.getErrors();
+
+  //   Options.
+  if (Options.FormatStyle) {
+    auto Style = format::getStyle(*Options.FormatStyle,
+                                  Filename.getSingleStringRef(), "LLVM");
+
+    llvm::Expected<tooling::Replacements> CleannedReplacements =
+        format::cleanupAroundReplacements(Code, Fixes, *Style);
+    if (!CleannedReplacements) {
+      llvm::errs() << llvm::toString(CleannedReplacements.takeError()) << "\n";
+      return "";
+    }
+    if (llvm::Expected<tooling::Replacements> FormattedReplacements =
+            format::formatReplacements(Code, *CleannedReplacements, *Style)) {
+      CleannedReplacements = std::move(FormattedReplacements);
+      if (!CleannedReplacements)
+        llvm_unreachable("!CleannedReplacements");
+    } else {
+      llvm::errs() << llvm::toString(FormattedReplacements.takeError())
+                   << ". Skipping formatting.\n";
+    }
+    auto Result = tooling::applyAllReplacements(Code, *CleannedReplacements);
+    if (!Result) {
+      // FIXME: propogate the error.
+      llvm::consumeError(Result.takeError());
+      return "";
+    }
+
+    return *Result;
+  } else {
     auto Result = tooling::applyAllReplacements(Code, Fixes);
     if (!Result) {
       // FIXME: propogate the error.
       llvm::consumeError(Result.takeError());
       return "";
     }
+
     return *Result;
   }
+}
+
+template <typename... CheckList>
+std::string runCheckAndFormatOnCode(
+    StringRef Code, StringRef FormatStyle = "llvm",
+    std::vector<ClangTidyError> *Errors = nullptr,
+    const Twine &Filename = "input.cc", ArrayRef<std::string> ExtraArgs = None,
+    const ClangTidyOptions &ExtraOptions = ClangTidyOptions(),
+    std::map<StringRef, StringRef> PathsToContent =
+        std::map<StringRef, StringRef>()) {
+
+  ClangTidyOptions Options = ExtraOptions;
+  Options.FormatStyle = FormatStyle;
+  return runCheckOnCode<CheckList...>(Code, Errors, Filename, ExtraArgs,
+                                      Options, PathsToContent);
+}
 
 #define EXPECT_NO_CHANGES(Check, Code)                                         \
   EXPECT_EQ(Code, runCheckOnCode<Check>(Code))
 
 } // namespace test
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANG_TIDY_CLANGTIDYTEST_H

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 {\n"
+            "  S() {}\n"
+            "  bool a_{false};\n"
+            "};",
+            runCheckAndFormatOnCode<UseDefaultMemberInitCheck>(
+                "struct S { S() : a_(false) {} bool a_; };"));
+}
+
+TEST(UseDefaultMemberInitCheckTest, SeveralInitializers) {
+  EXPECT_EQ(
+      "struct S {\n"
+      "  S() {}\n"
+      "  bool a_{false};\n"
+      "  bool b_{true};\n"
+      "};",
+      runCheckAndFormatOnCode<UseDefaultMemberInitCheck>(
+          "struct S { S() : a_(false), b_(true) {} bool a_; bool b_; };"));
+}
+
+TEST(UseDefaultMemberInitCheckTest, ExceptSpec) {
+  EXPECT_EQ(
+      "struct S {\n"
+      "  S() noexcept(true) {}\n"
+      "  bool a_{false};\n"
+      "  bool b_{true};\n"
+      "};",
+      runCheckAndFormatOnCode<UseDefaultMemberInitCheck>(
+          "struct S { S() noexcept(true) : a_(false), b_(true) {} bool a_; "
+          "bool b_; };"));
+  EXPECT_EQ(
+      "#define NOEXCEPT_(X) noexcept(X)\n"
+      "struct S {\n"
+      "  S() NOEXCEPT_(true) {}\n"
+      "  bool a_{false};\n"
+      "  bool b_{true};\n"
+      "};",
+      runCheckAndFormatOnCode<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 {\n"
+            "  S() {}\n"
+            "  bool a_{false};\n"
+            "  bool b_{true};\n"
+            "};",
+            runCheckAndFormatOnCode<UseDefaultMemberInitCheck>(
+                "struct S { S() : a_(false), b_(true) {} bool a_{false}; bool "
+                "b_{true}; };"));
+  EXPECT_EQ("struct S {\n"
+            "  S() : a_(true) {}\n"
+            "  bool a_{false};\n"
+            "  bool b_{true};\n"
+            "};",
+            runCheckAndFormatOnCode<UseDefaultMemberInitCheck>(
+                "struct S { S() : a_(true), b_(true) {} bool a_{false}; bool "
+                "b_{true}; };"));
+  EXPECT_EQ("struct S {\n"
+            "  S() : b_(false) {}\n"
+            "  bool a_{false};\n"
+            "  bool b_{true};\n"
+            "};",
+            runCheckAndFormatOnCode<UseDefaultMemberInitCheck>(
+                "struct S { S() : a_(false), b_(false) {} bool a_{false}; bool "
+                "b_{true}; };"));
+}
+
+} // namespace test
+} // namespace tidy
+} // namespace clang