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