clang-tidy: modernize-use-equals-default avoid adding redundant semicolons

[poelmanc]

Summary

modernize-use-equals-default replaces default constructors/destructors with = default;. When the optional semicolon after a member function is present, this results in two consecutive semicolons.

This patch checks to see if the next non-comment token after the code to be replaced is a semicolon, and if so offers a replacement of = default rather than = default;.

This patch adds trailing comments and semicolons to about 5 existing tests.

[JonasToth]

Please mention this improvement in the release notes as well :)

clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp

307

There should be utility code in clang-tidy/util/ that deals with custom lexing, that should be able to do the job.

[poelmanc]

clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp

307

Thanks for the quick feedback! I looked there and couldn't find a utility function that searched (a) forward rather than backwards, and (b) for any token not of a given list of types. Here's a new helper function that could be placed in LexerUtils.h if you think it's generally useful:

// Analogous to findNextAnyTokenKind, finds next token not of
// the given set of TokenKinds. Useful for skipping comments.
template <typename TokenKind, typename... TokenKinds>
Optional<Token> findNextTokenSkippingKind(SourceLocation Start,
                                          const SourceManager &SM,
                                          const LangOptions &LangOpts,
                                          TokenKind TK, TokenKinds... TKs) {
  Optional<Token> CurrentToken;
  do {
    Optional<Token> CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
  } while (CurrentToken && CurrentToken.isOneOf(TK, TKs...));
  return CurrentToken;
}

Then the do loop below can be simplified to:

Optional<Token> Token = findNextTokenSkippingKind(
        Body->getSourceRange().getEnd().getLocWithOffset(1),
        Result.Context->getSourceManager(), Result.Context->getLangOpts(),
        tok::comment);

[JonasToth]

clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp

307

There is findNextAnyTokenKind, not sure if it is actually practical to use in this case.

You can implement a skipping function as well. I think that is useful in other contexts (I recall that I had to do similiar skipping already at some point, not sure where it was :( )

[poelmanc]

Update to add and use new findNextTokenSkippingComments and findNextTokenSkippingKind utility functions. Since the former calls the latter with just one token type, this required generalizing Token::isOneOf() to work with 1-to-N token kinds versus requiring 2-to-N.

Also added a release note.

To me the change to Token::isOneOf() is an improvement (less code and more flexibility when calling it from variadic templates), but if the team prefers not to modify Token::isOneOf(), we could eliminate the findNextTokenSkippingKind utility function and instead implement findNextTokenSkippingComments directly.

[JonasToth]

Are you using the lexer-util functions? Maybe i am just to tired, but they seem to be unused.

clang/include/clang/Lex/Token.h

101 ↗

(On Diff #229161)

i would be rather against it. it is logically wrong as well, asl isOneOf() implies a choice.

[poelmanc]

Change to add just one helper function findNextTokenSkippingComments to LexerUtils.h, requiring no change to Token::isOneOf, and properly call it from UseEqualsDefaultCheck.cpp.

In D70144#1744737, @JonasToth wrote:

Are you using the lexer-util functions? Maybe i am just to tired, but they seem to be unused.

Sorry, that was not you! I was experimenting with the git show command to generate these patches and failed to check the results closely. The correct calling code should be in the updated diff.

[malcolm.parsons]

Should clang::format::cleanupAroundReplacements() handle this instead?

[JonasToth]

In D70144#1745532, @malcolm.parsons wrote:

Should clang::format::cleanupAroundReplacements() handle this instead?

Of yes. Totally forgot that. That would probably the most elegant way :)

[poelmanc]

In D70144#1745583, @JonasToth wrote:

In D70144#1745532, @malcolm.parsons wrote:

Should clang::format::cleanupAroundReplacements() handle this instead?

Of yes. Totally forgot that. That would probably the most elegant way :)

Interesting, so is the advantage of that approach that any fixit Replacement or Insertion that ends with a semicolon would have it removed if a semicolon already immediately follows it? That makes sense - one less thing for individual check developers to worry about.

I can take a look at doing that.

[JonasToth]

In D70144#1745881, @poelmanc wrote:

In D70144#1745583, @JonasToth wrote:

In D70144#1745532, @malcolm.parsons wrote:

Should clang::format::cleanupAroundReplacements() handle this instead?

Of yes. Totally forgot that. That would probably the most elegant way :)

Interesting, so is the advantage of that approach that any fixit Replacement or Insertion that ends with a semicolon would have it removed if a semicolon already immediately follows it? That makes sense - one less thing for individual check developers to worry about.

I can take a look at doing that.

Thinking about that: The cleanup should be called in general after clang-tidy runs. So it is weird, that it does not catch it.

[poelmanc]

! In D70144#1745583, @JonasToth wrote:

! In D70144#1745532, @malcolm.parsons wrote:

Should clang::format::cleanupAroundReplacements() handle this instead?

My initial attempt did not go well. I thought perhaps adding:

cleanupLeft(Line->First, tok::semi, tok::semi);

to clang/lib/Format/Format.cpp:1491 might do the trick. Stepping through that in the debugger shows that cleanupPair iterates over tokens on affected lines over the affected range. But after the newly added default token and subsequent semi token comes a nullptr - I could not see how to peek past the default; to see what else is on the line.

There's a comment admitting that this needs some architectural work:

// FIXME: in the current implementation the granularity of affected range
// is an annotated line. However, this is not sufficient. Furthermore,
// redundant code introduced by replacements does not necessarily
// intercept with ranges of replacements that result in the redundancy.
// To determine if some redundant code is actually introduced by
// replacements(e.g. deletions), we need to come up with a more
// sophisticated way of computing affected ranges.

Agreed. Even if we could see all the tokens on a line, it seems the test case I added to this patch at clang-tidy/checkers/modernize-use-equals-default.cpp:50, where the redundant semicolon is on the next line, could never be addressed given the current architecture. Changing the TokenAnalyzer::analyze() interface would affect JavaScriptRequoter, ObjCHeaderStyleGuesser, JavaScriptImportSorter, Formatter, and others.

Thoughts on how to proceed?

[JonasToth]

Hmm. I think this is fine, even though its not perfect.
@aaron.ballman wdyt?

[poelmanc]

Just a quick update, I made some progress addressing the architectural limitation of AnnotatedLines and evaluate(). I changed the AnnotatedLines constructor to also take a Line *PrevAnnotatedLine argument and set Line->First->Prev to the Last of the previous line and Line->Last->Next to the First of the next line. So now the Tokens remain connected across lines, so individual TokenAnalyzer subclasses can easily peek ahead and behind the current changed Line if they wish. Places that previously iterated e.g. while(Tok) had to be changed to iterate while(Tok && Tok != Line->Last->Next) - I probably haven't found all of those places yet.

The good news is, with that architectural change my prior addition of:

cleanupLeft(Line->First, tok::semi, tok::semi);

successfully removed redundant semicolons after default, even when the next semicolon was on the next line and/or separated by comments!

The bad news is this change also removed consecutive semicolons in e.g.:

for(<possible stuff
    here> ;; <maybe more here>)

which breaks some existing tests. I believe @MyDeveloperDay had thoughts on how to avoid replacing those meaningful consecutive semicolons? We have only tokens rather than an AST. We could search backwards for a for token, but I don't know how far backwards we would want to search to determine whether this particular ;; is valid.

In D70144#1748881, @JonasToth wrote:

Hmm. I think this is fine, even though its not perfect.
@aaron.ballman wdyt?

The attached patch is nice and small: a 5-line generally useful utility function plus a few lines to call it.

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp

 //===--- UseEqualsDefaultCheck.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
 //
 //===----------------------------------------------------------------------===//
 
 #include "UseEqualsDefaultCheck.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 const char SpecialFunction[] = "SpecialFunction";
void UseEqualsDefaultCheck::check(const MatchFinder::MatchResult &Result) {
   // expansion locations are reported.
   SourceLocation Location = SpecialFunctionDecl->getLocation();
   if (Location.isMacroID())
     Location = Body->getBeginLoc();
 
   auto Diag = diag(Location, "use '= default' to define a trivial " +
                                  SpecialFunctionName);
 
-  if (ApplyFix)
-    Diag << FixItHint::CreateReplacement(Body->getSourceRange(), "= default;")
+  if (ApplyFix) {
+    // Skipping comments, check for a semicolon after Body->getSourceRange()
+    Optional<Token> Token = utils::lexer::findNextTokenSkippingComments(
+        Body->getSourceRange().getEnd().getLocWithOffset(1),
+        Result.Context->getSourceManager(), Result.Context->getLangOpts());
+    StringRef Replacement =
+        Token && Token->is(tok::semi) ? "= default" : "= default;";
+    Diag << FixItHint::CreateReplacement(Body->getSourceRange(), Replacement)
          << RemoveInitializers;
   }
+}
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/utils/LexerUtils.h

while (true) {
     // the loop, otherwise we will get infinite loop (findNextToken will return
     // eof on eof).
     if (PotentialMatch.is(tok::eof))
       return SourceLocation();
     Start = PotentialMatch.getLastLoc();
   }
 }
 
+// Finds next token that's not a comment.
+Optional<Token> findNextTokenSkippingComments(SourceLocation Start,
+                                              const SourceManager &SM,
+                                              const LangOptions &LangOpts);
+
 /// Re-lex the provide \p Range and return \c false if either a macro spans
 /// multiple tokens, a pre-processor directive or failure to retrieve the
 /// next token is found, otherwise \c true.
 bool rangeContainsExpansionsOrDirectives(SourceRange Range,
                                          const SourceManager &SM,
                                          const LangOptions &LangOpts);
 
 /// Assuming that ``Range`` spans a const-qualified type, returns the ``const``

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

SourceLocation findPreviousTokenKind(SourceLocation Start,
   }
 }
 
 SourceLocation findNextTerminator(SourceLocation Start, const SourceManager &SM,
                                   const LangOptions &LangOpts) {
   return findNextAnyTokenKind(Start, SM, LangOpts, tok::comma, tok::semi);
 }
 
+Optional<Token> findNextTokenSkippingComments(SourceLocation Start,
+                                              const SourceManager &SM,
+                                              const LangOptions &LangOpts) {
+  Optional<Token> CurrentToken;
+  do {
+    CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
+  } while (CurrentToken && CurrentToken->is(tok::comment));
+  return CurrentToken;
+}
+
 bool rangeContainsExpansionsOrDirectives(SourceRange Range,
                                          const SourceManager &SM,
                                          const LangOptions &LangOpts) {
   assert(Range.isValid() && "Invalid Range for relexing provided");
   SourceLocation Loc = Range.getBegin();
 
   while (Loc < Range.getEnd()) {
     if (Loc.isMacroID())

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`bugprone-posix-return
   <clang-tidy/checks/bugprone-posix-return>` check.
 
   Now also checks if any calls to ``pthread_*`` functions expect negative return
   values.
 
 - The 'objc-avoid-spinlock' check was renamed to :doc:`darwin-avoid-spinlock
   <clang-tidy/checks/darwin-avoid-spinlock>`
 
+- The :doc:`modernize-use-equals-default
+  <clang-tidy/checks/modernize-use-equals-default>` fix no longer adds
+  semicolons where they would be redundant.
+
 - New :doc:`readability-redundant-access-specifiers
   <clang-tidy/checks/readability-redundant-access-specifiers>` check.
 
   Finds classes, structs, and unions that contain redundant member
   access specifiers.
 
 - New :doc:`readability-make-member-function-const
   <clang-tidy/checks/readability-make-member-function-const>` check.

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

 Empty &Empty::operator=(const Empty &Other) { return *this; }
 // CHECK-MESSAGES: :[[@LINE-1]]:15: 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) {}
+                        Field4(Other.Field4) {};
   // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use '= default'
   // CHECK-FIXES: BF(const BF &Other) {{$}}
   // CHECK-FIXES:                     = default;
   BF &operator=(const BF &);
 
   unsigned Field1 : 3;
   int : 7;
   char Field2 : 6;

clang-tools-extra/test/clang-tidy/checkers/modernize-use-equals-default.cpp

 // RUN: %check_clang_tidy %s modernize-use-equals-default %t -- -- -fno-delayed-template-parsing -fexceptions
 
 // Out of line definition.
 class OL {
 public:
   OL();
   ~OL();
 };
 
-OL::OL() {}
+OL::OL() {};
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use '= default' to define a trivial default constructor [modernize-use-equals-default]
 // CHECK-FIXES: OL::OL() = default;
 OL::~OL() {}
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use '= default' to define a trivial destructor [modernize-use-equals-default]
 // CHECK-FIXES: OL::~OL() = default;
 
 // Inline definitions.
 class IL {
 public:
-  IL() {}
+  IL() {}    ; // Note embedded tab on this line
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
-  // CHECK-FIXES: IL() = default;
+  // CHECK-FIXES: IL() = default   ; // Note embedded tab on this line
   ~IL() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
   // CHECK-FIXES: ~IL() = default;
 };
 
 // Non-empty body.
 void f();
 class NE {
public:
   // Constructor with arguments.
   IA(int Arg1, int Arg2) {}
   int Field;
 };
 
 // Default member initializer
 class DMI {
 public:
-  DMI() {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
-  // CHECK-FIXES: DMI() = default;
+  DMI() {} // Comment before semi-colon on next line
+  ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use '= default'
+  // CHECK-FIXES: DMI() = default // Comment before semi-colon on next line
+  // CHECK-FIXES-NEXT:   ;
   int Field = 5;
 };
 
 // Class member
 class CM {
 public:
-  CM() {}
+  CM() {} /* Comments */ /* before */ /* semicolon */;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
-  // CHECK-FIXES: CM() = default;
+  // CHECK-FIXES: CM() = default /* Comments */ /* before */ /* semicolon */;
   OL o;
 };
 
 // Private constructor/destructor.
 class Priv {
   Priv() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
   // CHECK-FIXES: Priv() = default;
-  ~Priv() {}
+  ~Priv() {};
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
   // CHECK-FIXES: ~Priv() = default;
 };
 
 // struct.
 struct ST {
   ST() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'