[clang-tidy] Simplify modernize-use-default

[malcolm.parsons]

Summary

clang-tidy now cleans up after replacements, so leave colon and comma
removal to that.

[malcolm.parsons]

test/clang-tidy/modernize-use-default-copy.cpp

85 ↗

(On Diff #75140)

I don't know why cleanup removes this comment, but there are format units tests that check that it does.

[aaron.ballman]

LGTM with a small nit, but you may want to wait for @djasper to weigh in on the removed comment question.

clang-tidy/modernize/UseDefaultCheck.cpp

266 ↗

(On Diff #75140)

You can use const auto * here.

[djasper]

I don't know whether it is an intentional choice to remove this comment. I'd be fine either way (I think there are arguments for and against it). So, this looks good to me. But maybe Eric has something to add.

[ioeric]

test/clang-tidy/modernize-use-default-copy.cpp

85 ↗

(On Diff #75140)

This is intended behavior of cleanup. Generally, if a deleted code results in a redundant token around it, comments between the redundant token and its previous/next token (ignoring the deleted code) are considered belonging to the deleted code. Considering how a normal developer writes code, this would be correct most of the time IMO.

clang-tools-extra/trunk/clang-tidy/modernize/UseDefaultCheck.cpp

 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 static const char SpecialFunction[] = "SpecialFunction";
 
-/// \brief Finds the SourceLocation of the colon ':' before the initialization
-/// list in the definition of a constructor.
-static SourceLocation getColonLoc(const ASTContext *Context,
-                                  const CXXConstructorDecl *Ctor) {
-  // FIXME: First init is the first initialization that is going to be
-  // performed, no matter what was the real order in the source code. If the
-  // order of the inits is wrong in the code, it may result in a false negative.
-  SourceLocation FirstInit = (*Ctor->init_begin())->getSourceLocation();
-  SourceLocation LastArg =
-      Ctor->getParamDecl(Ctor->getNumParams() - 1)->getLocEnd();
-  // We need to find the colon between the ')' and the first initializer.
-  bool Invalid = false;
-  StringRef Text = Lexer::getSourceText(
-      CharSourceRange::getCharRange(LastArg, FirstInit),
-      Context->getSourceManager(), Context->getLangOpts(), &Invalid);
-  if (Invalid)
-    return SourceLocation();
-
-  size_t ColonPos = Text.rfind(':');
-  if (ColonPos == StringRef::npos)
-    return SourceLocation();
-
-  Text = Text.drop_front(ColonPos + 1);
-  if (std::strspn(Text.data(), " \t\r\n") != Text.size()) {
-    // If there are comments, preprocessor directives or anything, abort.
-    return SourceLocation();
-  }
-  // FIXME: don't remove comments in the middle of the initializers.
-  return LastArg.getLocWithOffset(ColonPos);
-}
-
 /// \brief Finds all the named non-static fields of \p Record.
 static std::set<const FieldDecl *>
 getAllNamedFields(const CXXRecordDecl *Record) {
   std::set<const FieldDecl *> Result;
   for (const auto *Field : Record->fields()) {
     // Static data members are not in this range.
     if (Field->isUnnamedBitfield())
       continue;
Finder->addMatcher(
                       hasParameter(0, hasType(lValueReferenceType())))
             .bind(SpecialFunction),
         this);
   }
 }
 
 void UseDefaultCheck::check(const MatchFinder::MatchResult &Result) {
   std::string SpecialFunctionName;
-  SourceLocation StartLoc, EndLoc;
 
   // Both CXXConstructorDecl and CXXDestructorDecl inherit from CXXMethodDecl.
   const auto *SpecialFunctionDecl =
       Result.Nodes.getNodeAs<CXXMethodDecl>(SpecialFunction);
 
   // Discard explicitly deleted/defaulted special member functions and those
   // that are not user-provided (automatically generated).
   if (SpecialFunctionDecl->isDeleted() ||
       SpecialFunctionDecl->isExplicitlyDefaulted() ||
       SpecialFunctionDecl->isLateTemplateParsed() ||
       !SpecialFunctionDecl->isUserProvided() || !SpecialFunctionDecl->hasBody())
     return;
 
   const auto *Body = dyn_cast<CompoundStmt>(SpecialFunctionDecl->getBody());
   if (!Body)
     return;
 
-  // Default locations.
-  StartLoc = Body->getLBracLoc();
-  EndLoc = Body->getRBracLoc();
-
   // If there are comments inside the body, don't do the change.
   if (!SpecialFunctionDecl->isCopyAssignmentOperator() &&
       !bodyEmpty(Result.Context, Body))
     return;
 
+  std::vector<FixItHint> RemoveInitializers;
+
   if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(SpecialFunctionDecl)) {
     if (Ctor->getNumParams() == 0) {
       SpecialFunctionName = "default constructor";
     } else {
       if (!isCopyConstructorAndCanBeDefaulted(Result.Context, Ctor))
         return;
       SpecialFunctionName = "copy constructor";
       // If there are constructor initializers, they must be removed.
-      if (Ctor->getNumCtorInitializers() != 0) {
-        StartLoc = getColonLoc(Result.Context, Ctor);
-        if (!StartLoc.isValid())
-          return;
+      for (const CXXCtorInitializer *Init : Ctor->inits()) {
+        RemoveInitializers.emplace_back(
+            FixItHint::CreateRemoval(Init->getSourceRange()));
       }
     }
   } else if (isa<CXXDestructorDecl>(SpecialFunctionDecl)) {
     SpecialFunctionName = "destructor";
   } else {
     if (!isCopyAssignmentAndCanBeDefaulted(Result.Context, SpecialFunctionDecl))
       return;
     SpecialFunctionName = "copy-assignment operator";
   }
 
   diag(SpecialFunctionDecl->getLocStart(),
        "use '= default' to define a trivial " + SpecialFunctionName)
-      << FixItHint::CreateReplacement(
-          CharSourceRange::getTokenRange(StartLoc, EndLoc), "= default;");
+      << FixItHint::CreateReplacement(Body->getSourceRange(), "= default;")
+      << RemoveInitializers;
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/test/clang-tidy/modernize-use-default-copy.cpp

 // RUN: %check_clang_tidy %s modernize-use-default %t -- -- -std=c++11 -fno-delayed-template-parsing -fexceptions
 
 // Out of line definition.
 struct OL {
   OL(const OL &);
   OL &operator=(const OL &);
   int Field;
 };
 OL::OL(const OL &Other) : Field(Other.Field) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use '= default' to define a trivial copy constructor [modernize-use-default]
-// CHECK-FIXES: OL::OL(const OL &Other) = default;
+// CHECK-FIXES: OL::OL(const OL &Other)  = default;
 OL &OL::operator=(const OL &Other) {
   Field = Other.Field;
   return *this;
 }
 // CHECK-MESSAGES: :[[@LINE-4]]:1: warning: use '= default' to define a trivial copy-assignment operator [modernize-use-default]
 // CHECK-FIXES: OL &OL::operator=(const OL &Other) = default;
 
 // Inline.
 struct IL {
   IL(const IL &Other) : Field(Other.Field) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
-  // CHECK-FIXES: IL(const IL &Other) = default;
+  // CHECK-FIXES: IL(const IL &Other)  = default;
   IL &operator=(const IL &Other) {
     Field = Other.Field;
     return *this;
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use '= default'
   // CHECK-FIXES: IL &operator=(const IL &Other) = default;
   int Field;
 };
 
 // Wrong type.
 struct WT {
   WT(const IL &Other) {}
   WT &operator=(const IL &);
 };
 WT &WT::operator=(const IL &Other) { return *this; }
 
 // Qualifiers.
 struct Qual {
   Qual(const Qual &Other) : Field(Other.Field), Volatile(Other.Volatile),
                             Mutable(Other.Mutable), Reference(Other.Reference),
                             Const(Other.Const) {}
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use '= default'
-  // CHECK-FIXES: Qual(const Qual &Other) = default;
+  // CHECK-FIXES: Qual(const Qual &Other)
+  // CHECK-FIXES:                          = default;
 
   int Field;
   volatile char Volatile;
   mutable bool Mutable;
   const OL &Reference; // This makes this class non-assignable.
   const IL Const;      // This also makes this class non-assignable.
   static int Static;
 };
MF &MF::operator=(const MF &Other) {
   Field1 = Other.Field1;
   Field2 = Other.Field2;
   return *this;
 }
 
 struct Comments {
   Comments(const Comments &Other)
       /* don't delete */ : /* this comment */ Field(Other.Field) {}
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use '= default'
+  // CHECK-FIXES: /* don't delete */  = default;
   int Field;
 };
 
 struct MoreComments {
   MoreComments(const MoreComments &Other) /* this comment is OK */
       : Field(Other.Field) {}
   // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use '= default'
   // CHECK-FIXES: MoreComments(const MoreComments &Other) /* this comment is OK */
   // CHECK-FIXES-NEXT: = default;
   int Field;
 };
 
 struct ColonInComment {
   ColonInComment(const ColonInComment &Other) /* : */ : Field(Other.Field) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
-  // CHECK-FIXES: ColonInComment(const ColonInComment &Other) /* : */ = default;
+  // CHECK-FIXES: ColonInComment(const ColonInComment &Other) /* : */  = default;
   int Field;
 };
 
 // No members or bases (in particular, no colon).
 struct Empty {
   Empty(const Empty &Other) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
   // CHECK-FIXES: Empty(const Empty &Other) = default;
   Empty &operator=(const Empty &);
 };
 Empty &Empty::operator=(const Empty &Other) { return *this; }
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use '= default'
 // CHECK-FIXES: Empty &Empty::operator=(const Empty &Other) = default;
 
 // Bit fields.
 struct BF {
   BF() = default;
   BF(const BF &Other) : Field1(Other.Field1), Field2(Other.Field2), Field3(Other.Field3),
                         Field4(Other.Field4) {}
   // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use '= default'
-  // CHECK-FIXES: BF(const BF &Other) = default;
+  // CHECK-FIXES: BF(const BF &Other) {{$}}
+  // CHECK-FIXES:                     = default;
   BF &operator=(const BF &);
 
   unsigned Field1 : 3;
   int : 7;
   char Field2 : 6;
   int : 0;
   int Field3 : 24;
   unsigned char Field4;
 };
 BF &BF::operator=(const BF &Other) {
   Field1 = Other.Field1;
   Field2 = Other.Field2;
   Field3 = Other.Field3;
   Field4 = Other.Field4;
   return *this;
 }
 // CHECK-MESSAGES: :[[@LINE-7]]:1: warning: use '= default'
 // CHECK-FIXES: BF &BF::operator=(const BF &Other) = default;
 
 // Base classes.
 struct BC : IL, OL, BF {
   BC(const BC &Other) : IL(Other), OL(Other), BF(Other) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
-  // CHECK-FIXES: BC(const BC &Other) = default;
+  // CHECK-FIXES: BC(const BC &Other)  = default;
   BC &operator=(const BC &Other);
 };
 BC &BC::operator=(const BC &Other) {
   IL::operator=(Other);
   OL::operator=(Other);
   BF::operator=(Other);
   return *this;
 }
 // CHECK-MESSAGES: :[[@LINE-6]]:1: warning: use '= default'
 // CHECK-FIXES: BC &BC::operator=(const BC &Other) = default;
 
 // Base classes with member.
 struct BCWM : IL, OL {
   BCWM(const BCWM &Other) : IL(Other), OL(Other), Bf(Other.Bf) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
-  // CHECK-FIXES: BCWM(const BCWM &Other) = default;
+  // CHECK-FIXES: BCWM(const BCWM &Other)  = default;
   BCWM &operator=(const BCWM &);
   BF Bf;
 };
 BCWM &BCWM::operator=(const BCWM &Other) {
   IL::operator=(Other);
   OL::operator=(Other);
   Bf = Other.Bf;
   return *this;

 struct VA : virtual OL {};
 struct VB : virtual OL {};
 struct VBC : VA, VB, virtual OL {
   // OL is the first thing that is going to be initialized, despite the fact
   // that it is the last in the list of bases, because it is virtual and there
   // is a virtual OL at the beginning of VA (which is the same).
   VBC(const VBC &Other) : OL(Other), VA(Other), VB(Other) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
-  // CHECK-FIXES: VBC(const VBC &Other) = default;
+  // CHECK-FIXES: VBC(const VBC &Other)  = default;
   VBC &operator=(const VBC &Other);
 };
 VBC &VBC::operator=(const VBC &Other) {
   OL::operator=(Other);
   VA::operator=(Other);
   VB::operator=(Other);
   return *this;
 }

 }
 // CHECK-MESSAGES: :[[@LINE-5]]:1: warning: use '= default'
 // CHECK-FIXES: CIB &CIB::operator=(const CIB &Other) = default;
 
 // Take non-const reference as argument.
 struct NCRef {
   NCRef(NCRef &Other) : Field1(Other.Field1), Field2(Other.Field2) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
-  // CHECK-FIXES: NCRef(NCRef &Other) = default;
+  // CHECK-FIXES: NCRef(NCRef &Other)  = default;
   NCRef &operator=(NCRef &);
   int Field1, Field2;
 };
 NCRef &NCRef::operator=(NCRef &Other) {
   Field1 = Other.Field1;
   Field2 = Other.Field2;
   return *this;
 }