JonasToth added inline comments.
================
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:21
+namespace {
+
+// Skips any combination of temporary materialization, temporary binding and
----------------
I think you can elide the empty line around the matcher.
================
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:49
+ return;
+ // Look for calls to StrCat.
+ const auto StrCat = functionDecl(hasName("::absl::StrCat"));
----------------
Not sure if this comment adds value and might even be a little misleading.
`StrCat` itself does not look for calls, it is a helper to do so.
Maybe you can move the comment to the actual matching part with everything
composed and explain more what is specifically looked for.
================
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:51
+ const auto StrCat = functionDecl(hasName("::absl::StrCat"));
+ // The arguments of StrCat are implicitly converted to AlphaNum. Match that.
+ const auto AlphaNum = ignoringTemporaries(cxxConstructExpr(
----------------
Please make this comment a sentence. `Match that the arguments ...` would be ok
i think.
================
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:57
+
+ const auto has_another_reference_to_lhs =
+ callExpr(hasAnyArgument(expr(hasDescendant(declRefExpr(
----------------
Please rename this variable to follow the convention
================
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:79
+ const auto *Call = Result.Nodes.getNodeAs<CallExpr>("Call");
+
+ // Handles the case where x = absl::StrCat(x), which does nothing.
----------------
please add an `assert(Op != nullptr && Call != nullptr && "Matcher does not
work as expected")`, even though it seems trivial its better then nullptr deref.
And the logic might change in the future, so better be save
================
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:80
+
+ // Handles the case where x = absl::StrCat(x), which does nothing.
+ if (Call->getNumArgs() == 1) {
----------------
I think ` // Handles the case 'x = absl::StrCat(x)', which does nothing.`
would be better
================
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:82
+ if (Call->getNumArgs() == 1) {
+ diag(Op->getBeginLoc(), "call to absl::StrCat does nothing");
+ return;
----------------
please use 'absl::StrCat' in the diagnostics (same below) to signify that it is
code.
https://reviews.llvm.org/D51061
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
