mboehme added inline comments.
================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:37
@@ +36,3 @@
+ if (BeforeArgumentsRange.isValid() && AfterArgumentsRange.isValid()) {
+ (*Diag) << FixItHint::CreateRemoval(BeforeArgumentsRange)
+ << FixItHint::CreateRemoval(AfterArgumentsRange);
----------------
alexfh wrote:
> 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.
Thanks for the suggestion! If it's OK with you, though, I'd like to keep this
as-is -- I feel it makes the call-site clearer.
================
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:42
@@ +41,3 @@
+
+CharSourceRange FileCharRangeForExpr(const Expr *TheExpr,
+ const SourceManager &SM,
----------------
alexfh wrote:
> With two lines of implementation and just one caller, this function doesn't
> seem to add any value.
I've inlined the function. (I think I originally had two places where it got
called, so I factored it out, but then one of those places went away.)
================
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())) {
----------------
alexfh wrote:
> 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?
Actually, having looked into this some more, it seems that the macro cases are
already handled by the check "if (!FileMoveRange.isValid())" above -- so I'm
deleting this entire block of code.
(I think I added the isMacroBodyExpansion() and isMacroArgExpansion() checks
first before I moved the FileMoveRange.isValid() check, which then made them
unnecessary.)
http://reviews.llvm.org/D21223
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
