[PATCH] D75749: [clang-tidy] extend bugprone-signed-char-misuse check.

[Tamás Zolnai via Phabricator via cfe-commits]

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