[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:70-74
+  const auto CharExpr = expr(anyOf(
+      ignoringParenImpCasts(characterLiteral()),
+      declRefExpr(hasDeclaration(varDecl(hasType(qualType(isAnyCharacter()))))),
+      ignoringParens(callExpr(
+          callee(functionDecl(returns(qualType(isAnyCharacter()))))))));
----------------
add a testcase with:
```
char& chRef = ch;
std::string swapped2(chRef, i);

char* chPtr = &ch;
std::string swapped2(*chPtr, i);

using Character = char;
Character  c;
std::string swapped2(c, i);
```

I fear that it may not match those, because here you are trying to match specific use cases, when you should simply match a type of expression. In such case this should be just:
```
const auto CharExpr = expr(hasType(qualType(hasCanonicalType(anyOf(isAnyCharacter(), references(isAnyCharacter()))))));
```

Only issue would be implicit cast, you can try deal with them by using ignoringImplicit, or in better way just by using:

```
traverse(TK_IgnoreUnlessSpelledInSource,  hasArgument(0, CharExpr.bind("swapped-parameter"))),
```
TK_IgnoreUnlessSpelledInSource will cut of all implicit operations, so you could check if called constructor is actually the wrong one, and check argument types. Because now macher in line 125 may not always work if somehow we would have for example 2 implicit casts, for example from char reference to char, or some other crap.

It's up to you, but consider more generic approach instead of matching specific use cases like references to variables, or calls.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:95
+  const auto NonCharacterInteger =
+      qualType(isInteger(), unless(isAnyCharacter()));
+  const auto CharToIntCastExpr = implicitCastExpr(
----------------
what about references to integer, or typedefs to integers ?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:97
+  const auto CharToIntCastExpr = implicitCastExpr(
+      hasSourceExpression(expr(hasType(qualType(isAnyCharacter())))),
+      hasImplicitDestinationType(NonCharacterInteger));
----------------
reuse here CharExpr 


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:213
     }
+  } else if (const auto *E =
+                 Result.Nodes.getNodeAs<Expr>("implicit-cast-both-args")) {
----------------
i thing that this case should be matched by "swapped-parameter", no need to add extra one, just first one should be fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143971/new/

https://reviews.llvm.org/D143971