[PATCH] D71174: [clang-tidy] new check: bugprone-signed-char-misuse

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

ztamas marked 2 inline comments as done.
ztamas added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp:97-98
+  diag(CastExpression->getBeginLoc(),
+       "singed char -> integer (%0) conversion; "
+       "consider to cast to unsigned char first.")
+      << *IntegerType;
----------------
aaron.ballman wrote:
> How about: `'signed char' to %0 conversion; consider casting to 'unsigned char' first`?
> 
> Also, should we have a fix-it to automatically insert the cast to `unsigned char` in the proper location for the user?
I don't think it's a good idea to add a cast automatically. I think this kind of issue typically needs a human programmer to check what can be done in the code.
For example, when the program uses a char variable, but it is used as an int intentionally (without adding an int alias). It's not a good practice of course but it can happen. In this case, the best would be to use an int variable instead. Adding a cast might break the code if it's called with negative values.
It also can happen that the code works with actual characters, but handles the negative values on its own. In this case, it might be not enough to add an unsigned char cast, but it might be needed to adjust the surrounding code too.
All in all, based on my experience with the findings in the LibreOffice code base, I would not use an automatic correction here.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst:25
+See also:
+`STR34-C. Cast characters to unsigned char before converting to larger integer sizes
+<https://wiki.sei.cmu.edu/confluence/display/c/STR34-C.+Cast+characters+to+unsigned+char+before+converting+to+larger+integer+sizes>`_
----------------
aaron.ballman wrote:
> ztamas wrote:
> > aaron.ballman wrote:
> > > Should this check also be registered in the CERT module?
> > This check does not cover the whole CERT description. I guess a cert check should catch all bugous code related to the CERT issue, right?
> So long as this covers a decent amount of the CERT rule, I think it is fine to document the areas where we don't quite match the CERT rule.
My plan is to extend this check with other use cases in the next months. Can we get back to this question after that? Now, It's hard to tell how well it covers the CERT rule.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71174