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