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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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;
----------------
ztamas wrote:
> 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.
Okay, that makes sense, thank you!


================
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>`_
----------------
ztamas wrote:
> 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.
I checked and I think it covers the rule pretty closely. However, if you're already planning changes to cover new use cases, I'm fine waiting to add the alias.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:73
    `bugprone-posix-return <bugprone-posix-return.html>`_, "Yes", ""
+   `bugprone-signed-char-misuse <bugprone-signed-char-misuse.html>`_, , "medium"
    `bugprone-sizeof-container <bugprone-sizeof-container.html>`_, , "high"
----------------
Just an FYI, but the severity columns were recently removed, so you'll probably have to rebase your patch.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse-fsigned-char.cpp:6
+  int NCharacter = CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
----------------
I think you need to update all the diagnostic text in the tests as the diagnostic has changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71174