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