[PATCH] D17488: Extend UnnecessaryCopyInitialization check to trigger on non-const copies that can be safely converted to const references.

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/performance/UnnecessaryCopyInitialization.cpp:69
@@ +68,3 @@
+               : "the variable '%0' is copy-constructed from a const reference "
+                 "but "
+                 "is only used as const reference; consider making it a const "
----------------
nit: Clang-format doesn't reflow string literals 
 One way to fix this is to merge all the lines of this string literal and clang-format again.

================
Comment at: clang-tidy/performance/UnnecessaryCopyInitialization.h:19
@@ -18,3 +18,3 @@
 
-// A check that detects const local variable declarations that are copy
+// A check that detects local variable declarations that are copy
 // initialized with the const reference of a function call or the const
----------------
Maybe just "The check detects local variable declarations ...."?

Also, should the .rst file be updated as well?

================
Comment at: clang-tidy/utils/DeclRefExprUtils.h:21
@@ +20,3 @@
+/// \brief Returns true if all DeclRefExpr to the variable within Stmt do not
+/// modify it.
+/// Returns true if only const methods or operators are called on the variable
----------------
nit: There should be an empty line after the brief summary, IIRC. (But you can look at the doxygen output and see how this looks like.)

================
Comment at: clang-tidy/utils/FixItHintUtils.cpp:23
@@ +22,3 @@
+  if (!Token.is(tok::unknown))
+    AmpLocation = Token.getLocation().getLocWithOffset(Token.getLength());
+  return FixItHint::CreateInsertion(AmpLocation, "&");
----------------
`Lexer::getLocForEndOfToken` seems to be more suitable for this purpose.

================
Comment at: clang-tidy/utils/FixItHintUtils.h:21
@@ +20,3 @@
+/// \brief Creates fix to make VarDecl a reference by adding '&'.
+FixItHint createReferenceFix(const VarDecl &Var, ASTContext &Context);
+
----------------
Since this is now a library, we should make names clear and unambiguous. I'd pull a part of the name's meaning to the namespace and expand the name with the needed details. Maybe something like this:

  namespace clang {
  namespace tidy {
  namespace utils {
  namespace create_fixit {
  
  FixItHint changeVarDeclToReference(...);
  FixItHint changeVarDeclToConst(...);


Repository:
  rL LLVM

http://reviews.llvm.org/D17488