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