njames93 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),
----------------
ztamas wrote:
> 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.
Fair enough, in that case can you specify in the docs that you only check for
`==` and `!=` comparison. Currently they only say it looks for comparison
rather than maybe equality comparison.
Also point about `hasAnyOperatorName` still stands.
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
