[PATCH] D21223: [clang-tidy] misc-move-const-arg: Detect if result of std::move() is being passed as a const ref argument

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Thank you for working on this! A few more comments in addition to what Aaron has written.


================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:22
@@ +21,3 @@
+
+void ReplaceCallWithArg(const CallExpr *TheCallExpr, DiagnosticBuilder *Diag,
+                        const SourceManager &SM, const LangOptions &LangOpts) {
----------------
aaron.ballman wrote:
> Should pass `Diag` by reference rather than by pointer.
I'd prefer the parameter to be named `Call` instead of `TheCallExpr`.  Would make it more consistent with the rest of clang-tidy code.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:37
@@ +36,3 @@
+  if (BeforeArgumentsRange.isValid() && AfterArgumentsRange.isValid()) {
+    (*Diag) << FixItHint::CreateRemoval(BeforeArgumentsRange)
+            << FixItHint::CreateRemoval(AfterArgumentsRange);
----------------
Alternatively, this function could return a `llvm::SmallVector<FixItHint, 2>` (either empty or with the fixes) and the caller could just feed it to the result of `diag()`. Not much difference though, just pointing to this possibility.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:42
@@ +41,3 @@
+
+CharSourceRange FileCharRangeForExpr(const Expr *TheExpr,
+                                     const SourceManager &SM,
----------------
With two lines of implementation and just one caller, this function doesn't seem to add any value.

================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:116
@@ +115,3 @@
+    // and that appears to take precendence.
+    if (SM.isMacroBodyExpansion(CallMove->getLocStart()) ||
+        SM.isMacroArgExpansion(CallMove->getLocStart())) {
----------------
Is this equivalent to just ignoring all cases where `CallMove->getLocStart().isMacroID()`? If not, what's the subset of the cases that would be ignored by `isMacroID()`, but not with the current condition?


http://reviews.llvm.org/D21223