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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
