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

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

ztamas marked an inline comment as done.
ztamas added inline comments.
Herald added a subscriber: whisperity.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp:43-44
+
+  const auto SignedCharType = expr(hasType(qualType(
+      allOf(isAnyCharacter(), isSignedInteger(), unless(IntTypedef)))));
+
----------------
aaron.ballman wrote:
> Does this properly handle architectures where `char` is signed rather than unsigned? Or does this only handle the case where the user wrote `signed char`?
It catches plain char too. I tested on 64 bit linux where char is signed by default.
The funsigned-char and fsigned-char options can be used to override the default behavior. Added some new test cases and also extended the documentation to make this clear.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp:43-44
+
+  const auto SignedCharType = expr(hasType(qualType(
+      allOf(isAnyCharacter(), isSignedInteger(), unless(IntTypedef)))));
+
----------------
ztamas wrote:
> aaron.ballman wrote:
> > Does this properly handle architectures where `char` is signed rather than unsigned? Or does this only handle the case where the user wrote `signed char`?
> It catches plain char too. I tested on 64 bit linux where char is signed by default.
> The funsigned-char and fsigned-char options can be used to override the default behavior. Added some new test cases and also extended the documentation to make this clear.
Most of the catches I found in LibreOffice / LLVM code had `char` type.
I found isSignedCharDefault() method in LLVM code where clang handles platform differences, as I see, so I assume clang-tidy works based on that. With the compilation options, the char signedness can be controlled anyway, so I think this will be OK.
I could test only with a 64 bit linux. There plain char is caught by default, as I mentioned, and I could change this behavior with the -funsigned-char option.


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71174