jdemeule created this revision.
jdemeule added a reviewer: alexfh.
Herald added subscribers: cfe-commits, mgorny.
'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).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43500
Files:
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
clang-tidy/modernize/UseDefaultMemberInitCheck.h
unittests/clang-tidy/CMakeLists.txt
unittests/clang-tidy/ModernizerModuleTest.cpp
Index: unittests/clang-tidy/ModernizerModuleTest.cpp
===================================================================
--- /dev/null
+++ unittests/clang-tidy/ModernizerModuleTest.cpp
@@ -0,0 +1,64 @@
+#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
Index: unittests/clang-tidy/CMakeLists.txt
===================================================================
--- unittests/clang-tidy/CMakeLists.txt
+++ unittests/clang-tidy/CMakeLists.txt
@@ -12,6 +12,7 @@
IncludeInserterTest.cpp
GoogleModuleTest.cpp
LLVMModuleTest.cpp
+ ModernizerModuleTest.cpp
NamespaceAliaserTest.cpp
ObjCModuleTest.cpp
OverlappingReplacementsTest.cpp
@@ -29,6 +30,7 @@
clangTidyAndroidModule
clangTidyGoogleModule
clangTidyLLVMModule
+ clangTidyModernizeModule
clangTidyObjCModule
clangTidyReadabilityModule
clangTidyUtils
Index: clang-tidy/modernize/UseDefaultMemberInitCheck.h
===================================================================
--- clang-tidy/modernize/UseDefaultMemberInitCheck.h
+++ clang-tidy/modernize/UseDefaultMemberInitCheck.h
@@ -31,12 +31,18 @@
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
Index: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===================================================================
--- clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -12,12 +12,45 @@
#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:
@@ -140,7 +173,8 @@
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) {
@@ -173,7 +207,8 @@
hasInClassInitializer(anything()),
hasParent(recordDecl(isUnion()))))),
isWritten(), withInitializer(ignoringImplicit(Init)))
- .bind("default"))),
+ .bind("default")))
+ .bind("ctor"),
this);
Finder->addMatcher(
@@ -183,23 +218,26 @@
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();
@@ -227,19 +265,71 @@
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;
- diag(Init->getSourceLocation(), "member initializer for %0 is redundant")
- << Field
- << FixItHint::CreateRemoval(Init->getSourceRange());
+ auto Diag =
+ diag(Init->getSourceLocation(), "member initializer for %0 is redundant")
+ << 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
