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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
