[clang-tidy] performance-unnecessary-copy-initialization: Do not remove comments on new lines.

[flx]

Summary

When deleting the copy assignment statement because copied variable is not used
only remove trailing comments on the same line.

[ymandel]

Given that you're removing a declaration, consider using tooling::getAssociatedRange https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/clang/include/clang/Tooling/Transformer/SourceCode.h;l=44 It does have a different semantics than the simpler logic that you have here, but, its based on code in a dead-code removal tool, so it may still be what you want.

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp

59

what happens for the last line in the file? I think the Offset + 1 below will be sketchy (because PastNewLine will be invalid). It might work, but seems best avoided.

64

I suspect this could get tripped up by macros and other expansions. Instead of std::min, consider using SourceManager::isBeforeInTranslationUnit
(clang/include/clang/Basic/SourceManager.h;l=1616)? Alternatively, check that both locations are file ids before calling std::min.

[flx]

Make offset operations safer.

[flx]

Thanks for the review!

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp

59

I added a check to not go past the end of the string.

64

We don't apply any fixes when the decl statement is inside of a macro, but switched to using SourceManager::isBeforeInTranslationUnit.

[ymandel]

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp

50

optional nit: invert the condition and the branches. i just find it a little easier to read positive predicates. So, when i have both branches, I try for the condition to be positive (or, in other words, I try not to not the condition. ;) )

[flx]

Invert end of string logic.

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp

void recordFixes(const VarDecl &Var, ASTContext &Context,
   Diagnostic << utils::fixit::changeVarDeclToReference(Var, Context);
   if (!Var.getType().isLocalConstQualified()) {
     if (llvm::Optional<FixItHint> Fix = utils::fixit::addQualifierToVarDecl(
             Var, Context, DeclSpec::TQ::TQ_const))
       Diagnostic << *Fix;
   }
 }
 
+llvm::Optional<SourceLocation> firstLocAfterNewLine(SourceLocation Loc,
+                                                    SourceManager &SM) {
+  bool Invalid;
+  const char *TextAfter = SM.getCharacterData(Loc, &Invalid);
+  if (Invalid) {
+    return llvm::None;
+  }
+  size_t Offset = std::strcspn(TextAfter, "\n");
+  return Loc.getLocWithOffset(TextAfter[Offset] == '\0' ? Offset : Offset + 1);
+}
+
 void recordRemoval(const DeclStmt &Stmt, ASTContext &Context,
                    DiagnosticBuilder &Diagnostic) {
-  // Attempt to remove the whole line until the next non-comment token.
-  auto Tok = utils::lexer::findNextTokenSkippingComments(
-      Stmt.getEndLoc(), Context.getSourceManager(), Context.getLangOpts());
-  if (Tok) {
-    Diagnostic << FixItHint::CreateRemoval(SourceRange(
-        Stmt.getBeginLoc(), Tok->getLocation().getLocWithOffset(-1)));
+  auto &SM = Context.getSourceManager();
+  // Attempt to remove trailing comments as well.
+  auto Tok = utils::lexer::findNextTokenSkippingComments(Stmt.getEndLoc(), SM,
+                                                         Context.getLangOpts());
+  llvm::Optional<SourceLocation> PastNewLine =
+      firstLocAfterNewLine(Stmt.getEndLoc(), SM);
+  if (Tok && PastNewLine) {
+    auto BeforeFirstTokenAfterComment = Tok->getLocation().getLocWithOffset(-1);
+    // Remove until the end of the line or the end of a trailing comment which
+    // ever comes first.
+    auto End =
+        SM.isBeforeInTranslationUnit(*PastNewLine, BeforeFirstTokenAfterComment)
+            ? *PastNewLine
+            : BeforeFirstTokenAfterComment;
+    Diagnostic << FixItHint::CreateRemoval(
+        SourceRange(Stmt.getBeginLoc(), End));
   } else {
     Diagnostic << FixItHint::CreateRemoval(Stmt.getSourceRange());
   }
 }
 
 AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningMethodCall) {
   // Match method call expressions where the `this` argument is only used as
   // const, this will be checked in `check()` part. This returned const

clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp

 void positiveUnusedReferenceIsRemoved() {
   // clang-format off
   const auto AutoAssigned = ExpensiveTypeReference(); int i = 0; // Foo bar.
   // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' is copy-constructed from a const reference but is never used; consider removing the statement [performance-unnecessary-copy-initialization]
   // CHECK-FIXES-NOT: const auto AutoAssigned = ExpensiveTypeReference();
   // CHECK-FIXES: int i = 0; // Foo bar.
   auto TrailingCommentRemoved = ExpensiveTypeReference(); // Trailing comment.
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'TrailingCommentRemoved' is copy-constructed from a const reference but is never used;
-  // CHECK-FIXES-NOT: auto TrailingCommentRemoved = ExpensiveTypeReference(); // Trailing comment.
+  // CHECK-FIXES-NOT: auto TrailingCommentRemoved = ExpensiveTypeReference();
+  // CHECK-FIXES-NOT: // Trailing comment.
   // clang-format on
+
+  auto UnusedAndUnnecessary = ExpensiveTypeReference();
+  // Comments on a new line should not be deleted.
+  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: the variable 'UnusedAndUnnecessary' is copy-constructed
+  // CHECK-FIXES-NOT: auto UnusedAndUnnecessary = ExpensiveTypeReference();
+  // CHECK-FIXES: // Comments on a new line should not be deleted.
 }
 
 void negativeloopedOverObjectIsModified() {
   ExpensiveToCopyType Orig;
   for (const auto &Element : Orig) {
     const auto Copy = Element;
     Orig.nonConstMethod();
     Copy.constMethod();