[clang-tidy] non-portable-integer-constant check

[ahmed]

Summary

According to
https://wiki.sei.cmu.edu/confluence/display/c/INT17-C.+Define+integer+constants+in+an+implementation-independent+manner

bugprone-non-portable-integer-constant check is created
The check finds masks that are being used in a non-portable manner.

[ahmed]

adding alias doc in ReleaseNotes.rst

[ahmed]

fix alias doc

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.cpp

1

Please make length 80 symbols.

25

Unnecessary empty line.

45

Unnecessary empty line.

49

Unnecessary empty line.

71

Please add newline.

clang-tools-extra/docs/ReleaseNotes.rst

75

Wrong symbol.

114

Please use alphabetical order fore new checks entries.

122

See previous release for such lines wrapping.

clang-tools-extra/docs/clang-tidy/checks/bugprone-non-portable-integer-constant.rst

4

Please make same length as title.

8

Should be C/C++. See other documentation.

15

Please add newline.

clang-tools-extra/docs/clang-tidy/checks/cert-int17-c.rst

6

Please make same length as title.

[aaron.ballman]

Have you run this check over any large code bases to see just how often it triggers and whether the diagnostics are false positives or true positives? I have a sneaking suspicion that this will yield a *ton* of false positives and probably not a whole lot of true positives. For starters, the issue isn't really a bugprone one -- the recommendation is about the behavior when compiling for different architectures or with different compilers, so this is more of a portability issue. It may make sense to move this into the portability module instead. However, the other issue is: a lot of CERT recommendations are recommendations rather than rules because it's impossible to determine programmer intent from the source code. If the user is intentionally using these literals, how should they silence this diagnostic?

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

38

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)

clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.cpp

41

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.

48

No need to test for this -- the only way check() gets called is if your matcher matches, and you only bind one matcher node, so this will always be nonnull. Feel free to assert it if you want.

55

Why only hex literals? Why not octal or binary literals, for instance?

64

There's extra whitespace in the diagnostic that should be removed, but also, this doesn't really tell the user how the integer is being used in a non-portable manner. If I saw that diagnostic, I wouldn't know how to respond to it to correct the issue.

clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.h

1

When you update the file names, be sure to hit things like these comments too.

clang-tools-extra/docs/ReleaseNotes.rst

75

It looks like a whitespace change snuck in and can be removed.

clang-tools-extra/docs/clang-tidy/checks/bugprone-non-portable-integer-constant.rst

6

This needs a lot more explanation, and it should also link back to the CERT recommendation somewhere.

[whisperity]

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

38

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)

124

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.

clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.cpp

21–23

24–38

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.

30

41

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?)

63–64

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 FixIts based off of that.

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

106–108

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?

clang-tools-extra/docs/ReleaseNotes.rst

115–117

The name of the check versus the one-liner summary doesn't really match up.

[steakhal]

clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.cpp

54

You are assigning a pointer to a temporal std::string. It will dangle.

58

There could be digit separator apostrophes. E.g.: 0x44'4'4.

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedPointerArithmeticInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "NoEscapeCheck.h"
+#include "NonportableintegerconstantCheck.h"
 #include "NotNullTerminatedResultCheck.h"
 #include "ParentVirtualCallCheck.h"
 #include "PosixReturnCheck.h"
 #include "RedundantBranchConditionCheck.h"
 #include "ReservedIdentifierCheck.h"
 #include "SignedCharMisuseCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MisplacedPointerArithmeticInAllocCheck>(
         "bugprone-misplaced-pointer-arithmetic-in-alloc");
     CheckFactories.registerCheck<MisplacedWideningCastCheck>(
         "bugprone-misplaced-widening-cast");
     CheckFactories.registerCheck<MoveForwardingReferenceCheck>(
         "bugprone-move-forwarding-reference");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
+    CheckFactories.registerCheck<NonportableintegerconstantCheck>(
+        "bugprone-non-portable-integer-constant");
     CheckFactories.registerCheck<RedundantBranchConditionCheck>(
         "bugprone-redundant-branch-condition");
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
         "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<NoEscapeCheck>("bugprone-no-escape");
     CheckFactories.registerCheck<NotNullTerminatedResultCheck>(
         "bugprone-not-null-terminated-result");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   MacroRepeatedSideEffectsCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MisplacedPointerArithmeticInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp
   MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
   NoEscapeCheck.cpp
   NotNullTerminatedResultCheck.cpp
+  NonportableintegerconstantCheck.cpp
   ParentVirtualCallCheck.cpp
   PosixReturnCheck.cpp
   RedundantBranchConditionCheck.cpp
   ReservedIdentifierCheck.cpp
   SignedCharMisuseCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   SpuriouslyWakeUpFunctionsCheck.cpp

clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.h

+//===--- NonportableintegerconstantCheck.h - clang-tidy ---------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NON_PORTABLE_INTEGER_CONSTANT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NON_PORTABLE_INTEGER_CONSTANT_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds masks that are being used in a nonportable manner
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-non-portable-integer-constant.html
+class NonportableintegerconstantCheck : public ClangTidyCheck {
+public:
+  NonportableintegerconstantCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NON_PORTABLE_INTEGER_CONSTANT_H

clang-tools-extra/clang-tidy/bugprone/NonportableintegerconstantCheck.cpp

+//===--- NonportableintegerconstantCheck.cpp - clang-tidy ----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "NonportableintegerconstantCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// 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`.
+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.
+    HexNumber = HexNumber.drop_while([](char C) { return C == '0'; });
+    return HexNumber.empty();
+  }
+
+  // Check whether all the bits are set to one by consuming all the 'F's.
+  HexNumber = HexNumber.drop_while([](char C) { return C == 'F' || C == 'f'; });
+  return HexNumber.empty();
+}
+
+void NonportableintegerconstantCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(integerLiteral().bind("integer"), this);
+}
+
+void NonportableintegerconstantCheck::check(const MatchFinder::MatchResult &Result) {
+
+  const auto *MatchedInt = Result.Nodes.getNodeAs<IntegerLiteral>("integer");
+
+  if (MatchedInt) {
+
+    StringRef MaskStr = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(MatchedInt->getSourceRange()),
+        *Result.SourceManager, Result.Context->getLangOpts(), 0);
+
+    MaskStr = StringRef(MaskStr.lower());
+    if (!MaskStr.consume_front("0x"))
+      return;
+
+    MaskStr = MaskStr.take_while(llvm::isHexDigit);
+
+    if (!isProblematicMask(MaskStr))
+      return;
+
+    diag(MatchedInt->getBeginLoc(),
+         "integer is being used in a non-portable manner ");
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
 No newline at end of file

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

 //===--- CERTTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../bugprone/BadSignalToKillThreadCheck.h"
+#include "../bugprone/NonPortableIntegerConstant.h"
 #include "../bugprone/ReservedIdentifierCheck.h"
 #include "../bugprone/SignedCharMisuseCheck.h"
 #include "../bugprone/SpuriouslyWakeUpFunctionsCheck.h"
 #include "../bugprone/UnhandledSelfAssignmentCheck.h"
 #include "../google/UnnamedNamespaceInHeaderCheck.h"
 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"
 #include "../misc/StaticAssertCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     // ENV
     CheckFactories.registerCheck<CommandProcessorCheck>("cert-env33-c");
     // FLP
     CheckFactories.registerCheck<FloatLoopCounter>("cert-flp30-c");
     // FIO
     CheckFactories.registerCheck<misc::NonCopyableObjectsCheck>("cert-fio38-c");
     // ERR
     CheckFactories.registerCheck<StrToNumCheck>("cert-err34-c");
+    // INT
+    CheckFactories.registerCheck<bugprone::NonPortableIntegerConstant>(
+        "cert-int17-c");
     // MSC
     CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc30-c");
     CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
         "cert-msc32-c");
     // POS
     CheckFactories.registerCheck<bugprone::BadSignalToKillThreadCheck>(
         "cert-pos44-c");
     // STR

clang-tools-extra/docs/ReleaseNotes.rst

 Improvements to clang-tidy
 --------------------------
 
 - Checks that allow configuring names of headers to include now support wrapping
   the include in angle brackets to create a system include. For example,
   :doc:`cppcoreguidelines-init-variables
   <clang-tidy/checks/cppcoreguidelines-init-variables>` and
   :doc:`modernize-make-unique <clang-tidy/checks/modernize-make-unique>`.
   
 New modules
 ^^^^^^^^^^^
 
 - New ``altera`` module.
 
   Includes checks related to OpenCL for FPGA coding guidelines, based on the
   `Altera SDK for OpenCL: Best Practices Guide
   <https://www.altera.com/en_US/pdfs/literature/hb/opencl-sdk/aocl_optimization_guide.pdf>`_.
- New :doc:`bugprone-redundant-branch-condition
 
   Finds condition variables in nested ``if`` statements that were also checked
   in the outer ``if`` statement and were not changed.
 
 - New :doc:`readability-function-cognitive-complexity
   <clang-tidy/checks/readability-function-cognitive-complexity>` check.
 
   Flags functions with Cognitive Complexity metric exceeding the configured limit.
  
+- New :doc:`bugprone-non-portable-integer-constant  
+  <clang-tidy/checks/bugprone-non-portable-integer-constant>` check.  
+  
+  Finds masks that are being used in a non-portable manner. 
+
+New check aliases
+^^^^^^^^^^^^^^^^^
+
+- New alias :doc:`cert-int17-c
+  <clang-tidy/checks/cert-int17-c>` to
+  :doc:`bugprone-non-portable-integer-constant
+  <clang-tidy/checks/bugprone-non-portable-integer-constant>` was added.
+
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`modernize-loop-convert
   <clang-tidy/checks/modernize-loop-convert>` check.
 
   Now able to transform iterator loops using ``rbegin`` and ``rend`` methods.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone-non-portable-integer-constant.rst

+.. title:: clang-tidy - bugprone-non-portable-integer-constant
+
+bugprone-non-portable-integer-constant
+===================================
+
+Finds masks that are being used in a non-portable manner.
+
+.. code-block:
+
+    const unsigned long mask = 0xFFFFFFFF;
+    
+    unsigned long flipbits(unsigned long x) {
+      return x ^ mask;
+    }
 No newline at end of file

clang-tools-extra/docs/clang-tidy/checks/cert-int17-c.rst

+.. title:: clang-tidy - cert-int17-c
+.. meta::
+   :http-equiv=refresh: 5;URL=bugprone-non-portable-integer-constant.html
+
+cert-int17-c
+==============
+
+The cert-int17-c check is an alias, please see
+`bugprone-non-portable-integer-constant <bugprone-non-portable-integer-constant.html>`_
+for more information.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `bugprone-macro-parentheses <bugprone-macro-parentheses.html>`_, "Yes"
    `bugprone-macro-repeated-side-effects <bugprone-macro-repeated-side-effects.html>`_,
    `bugprone-misplaced-operator-in-strlen-in-alloc <bugprone-misplaced-operator-in-strlen-in-alloc.html>`_, "Yes"
    `bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone-misplaced-pointer-arithmetic-in-alloc.html>`_, "Yes"
    `bugprone-misplaced-widening-cast <bugprone-misplaced-widening-cast.html>`_,
    `bugprone-move-forwarding-reference <bugprone-move-forwarding-reference.html>`_, "Yes"
    `bugprone-multiple-statement-macro <bugprone-multiple-statement-macro.html>`_,
    `bugprone-no-escape <bugprone-no-escape.html>`_,
+   `bugprone-non-portable-integer-constant <bugprone-non-portable-integer-constant.html>`_,
    `bugprone-not-null-terminated-result <bugprone-not-null-terminated-result.html>`_, "Yes"
    `bugprone-parent-virtual-call <bugprone-parent-virtual-call.html>`_, "Yes"
    `bugprone-posix-return <bugprone-posix-return.html>`_, "Yes"
    `bugprone-redundant-branch-condition <bugprone-redundant-branch-condition.html>`_, "Yes"
    `bugprone-reserved-identifier <bugprone-reserved-identifier.html>`_, "Yes"
    `bugprone-signed-char-misuse <bugprone-signed-char-misuse.html>`_,
    `bugprone-sizeof-container <bugprone-sizeof-container.html>`_,
    `bugprone-sizeof-expression <bugprone-sizeof-expression.html>`_,
.. csv-table::
    `cert-dcl50-cpp <cert-dcl50-cpp.html>`_,
    `cert-dcl58-cpp <cert-dcl58-cpp.html>`_,
    `cert-env33-c <cert-env33-c.html>`_,
    `cert-err34-c <cert-err34-c.html>`_,
    `cert-err52-cpp <cert-err52-cpp.html>`_,
    `cert-err58-cpp <cert-err58-cpp.html>`_,
    `cert-err60-cpp <cert-err60-cpp.html>`_,
    `cert-flp30-c <cert-flp30-c.html>`_,
+   `cert-int17-c <cert-int17-c.html>`_,
    `cert-mem57-cpp <cert-mem57-cpp.html>`_,
    `cert-msc50-cpp <cert-msc50-cpp.html>`_,
    `cert-msc51-cpp <cert-msc51-cpp.html>`_,
    `cert-oop57-cpp <cert-oop57-cpp.html>`_,
    `cert-oop58-cpp <cert-oop58-cpp.html>`_,
    `clang-analyzer-core.DynamicTypePropagation <clang-analyzer-core.DynamicTypePropagation.html>`_,
    `clang-analyzer-core.uninitialized.CapturedBlockVariable <clang-analyzer-core.uninitialized.CapturedBlockVariable.html>`_,
    `clang-analyzer-cplusplus.InnerPointer <clang-analyzer-cplusplus.InnerPointer.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone-non-portable-integer-constant.cpp

+// RUN: %check_clang_tidy %s bugprone-non-portable-integer-constant %t
+
+// #define ULONG_MAX 0xFFFFFFFFUL
+#include <limits.h>
+#include <stdio.h>
+
+const unsigned long mask = 0xFFFFFFFF;
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: integer is being used in a non-portable manner 
+
+const unsigned long mask1 = 0xffffffffUL;
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: integer is being used in a non-portable manner
+
+const unsigned long mask2 = 0x80000000;
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: integer is being used in a non-portable manner 
+
+int stuff() {
+  int mask3 = 0xff;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: integer is being used in a non-portable manner
+  int value = 0x55;
+  return mask3;
+}
+
+const unsigned long g = 0xC0;
+// no warnings
+
+const unsigned long x = -1;
+// no warnings
+
+const unsigned long y = ~(ULONG_MAX >> 1);
+// no warnings
+
+const unsigned long z = 0x11111111;
+// no warnings