[PATCH] D51061: [clang-tidy] abseil-str-cat-append

[Jonas Toth via Phabricator via cfe-commits]

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