ztamas marked an inline comment as done.
ztamas added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp:97
+ const auto CompareOperator =
+ expr(binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ anyOf(allOf(hasLHS(SignedCharCastExpr),
----------------
njames93 wrote:
> `binaryOperator(hasAnyOperatorName("==", "!=") ...`
> Also whats the reason for not checking `<`, `>`, `<=`, `>=` or `<=>`?
I think these are different cases when the programmer checks the equality of
two characters or checking some kind of order of characters. In case of
equality, there seems to be a clear assumption, that if the two character
variables represent the same character (semantically the same), then the
equality should return true. This assumption fails with mixed signed and
unsigned char variables.
However, when the programmer uses a less or a greater operator it's a bit
different. I guess a programmer in the case starts to wonder what is the actual
order of the characters, checks the ASCII table if it's not obvious for a set
of characters. Also, it's not clear what might be the false assumption here. Is
an ASCII character smaller or greater than a non-ASCII character? So I don't
see a probable false assumption here what we can point out. At least, in
general.
Furthermore, I optimized the check for equality/inequality, with ignoring the
ASCII characters for both the signed and the unsigned operand. This makes sense
for equality/inequality but does not make sense for the other comparison
operators.
All-in-all I'm now focusing on the equality / unequality operators. Usage of
these operands clearly something we can catch. Checking other comparison
operators is something that needs consideration, so I don't bother with that in
this patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75749/new/
https://reviews.llvm.org/D75749
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
