[PATCH] D90399: [clang-tidy] non-portable-integer-constant check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.
Herald added a subscriber: cfe-commits.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:38
 #include "NoEscapeCheck.h"
+#include "NonportableintegerconstantCheck.h"
 #include "NotNullTerminatedResultCheck.h"
----------------
aaron.ballman wrote:
> Please use PascalCase for this -- `NonPortableIntegerConstantCheck.h`  (be sure to change the actual case of the files being added in addition to the include directives)
> Please use PascalCase for this -- `NonPortableIntegerConstantCheck.h`  (be sure to change the actual case of the files being added in addition to the include directives)




================
Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:124
         "bugprone-multiple-statement-macro");
+    CheckFactories.registerCheck<NonportableintegerconstantCheck>(
+        "bugprone-non-portable-integer-constant");
----------------
> Please use PascalCase for this -- `NonPortableIntegerConstantCheck.h`  (be sure to change the actual case of the files being added in addition to the include directives)

Not just for the filename, but for the class's name as well, everywhere.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.cpp:21-23
+/// The mask is problematic if:
+/// - The highest bit is set and everything else are zeroes, for example: `0x80000000`.
+/// - All the bits are set to one, for example: `0xFFFFFFFF`.
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.cpp:24-38
+bool isProblematicMask(StringRef HexNumber) {
+
+ // Check whether only the highest bit is set.
+  if (HexNumber[0] == '8' && HexNumber[1] == '0') {
+    // Consume the first non-zero.
+    HexNumber = HexNumber.drop_front();
+    // Consume all the left zeroes.
----------------
C++17 introduced binary literals, i.e. `0b00001100`, which could also be used for masking operations. I suggest creating two functions `isUnsafeHexMask()` and `isUnsafeBinMask()` and a wrapper function that calls both(?), and supporting finding literals like `0b1000000000000000` and `0b1111111111111111`.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.cpp:30
+    HexNumber = HexNumber.drop_front();
+    // Consume all the left zeroes.
+    HexNumber = HexNumber.drop_while([](char C) { return C == '0'; });
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.cpp:41
+void NonportableintegerconstantCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(integerLiteral().bind("integer"), this);
+}
----------------
aaron.ballman wrote:
> I'm a bit worried about the performance of this check given that it's going to make a function call on every single integer literal anywhere in the program. I'd be curious to know how this check performs compared to other checks in the bugprone module.
I did a quick skim and it seems there is plenty of `integerLiteral(` matches used here and there, but most of them seem to either do an `equal(0)` or `equal(1)` sub-matching first. If performance is a concern, I think we could do an `unless(equal(0))`-like construct, given that no pattern that we try to check may match a literal 0.

(Although if we dig deeper into the maths, maybe we can come up with a better lower bound?)


================
Comment at: clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.cpp:63-64
+
+    diag(MatchedInt->getBeginLoc(),
+         "integer is being used in a non-portable manner ");
+  }
----------------
This message is misleading. It should emphasise that it looks like a bit mask, and given that the CERT rule suggests portable ways (`-1` instead of `0xFFFF` and `~(MAX >> 1)` instead of `0x8000`), this check should be able to produce //FixIt//s based off of that.


================
Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:106-108
+    // INT
+    CheckFactories.registerCheck<bugprone::NonPortableIntegerConstant>(
+        "cert-int17-c");
----------------
This is a C-specific check, or does it apply to C++ too? If only applies to C, it should be disabled in C++ mode, if it applies to C++, is there a C++-specific CERT rule/tag/number/ID for that version?


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:115-117
+  <clang-tidy/checks/bugprone-non-portable-integer-constant>` check.	
+  
+  Finds masks that are being used in a non-portable manner.	
----------------
The name of the check versus the one-liner summary doesn't really match up.


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

https://reviews.llvm.org/D90399