[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:28
+  for (;;) {
+    if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(E)) {
+      E = MTE->getTemporary();
----------------
You can ellide the braces for the single stmt ifs


================
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:52
+
+  const std::string str_cat = "::absl::StrCat";
+  const std::string alpha_num = "::absl::AlphaNum";
----------------
The naming is ambigous. `str_cat` can easily be mixed with `strcat`. Is this constant even necessary or could you use the literal in the functionDecl matcher?


================
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:85
+
+  // x = StrCat(x) does nothing.
+  if (call->getNumArgs() == 1) {
----------------
Please make this comment a full sentence


================
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:87
+  if (call->getNumArgs() == 1) {
+    diag(op->getBeginLoc(), "call to StrCat does nothing");
+    return;
----------------
I think it would be better to include `absl::StrCat` in the diagnostic, as it is more clear.
Same opinion on the other diag.


================
Comment at: test/clang-tidy/abseil-str-cat-append.cpp:91
+
+void bar() {
+  std::string a, b;
----------------
What happens if `StrCat` is used e.g. in an `std::accumulate` call as the binary operator? (https://en.cppreference.com/w/cpp/algorithm/accumulate  the Example includes such a case)
Is this diagnosed, too?

The general case would be if `StrCat` is used as a template argument for a functor.


================
Comment at: test/clang-tidy/abseil-str-cat-append.cpp:114
+
+}  // namespace absl
----------------
Could you please add a testcase that is outside of the `absl` namespace.

```
void f() {
  std::string s;
  s = absl::StrCat(s,s);
}
```

and 
```
void g() {
  std::string s;
  using absl::StrCat;
  s = StrCat(s, s);
}
```

with better naming.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51061