[PATCH] D146712: [clang-tidy] Add portability-non-portable-integer-constant check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.cpp:92
+void NonPortableIntegerConstantCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(integerLiteral().bind("integer"), this);
+}
----------------
consider excluding system headers, or even exclude things like "equals(0)" to speed up  check.


================
Comment at: clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.cpp:103
+  QualType IntegerLiteralType = MatchedInt->getType();
+  auto LiteralBitWidth = Result.Context->getTypeSize( IntegerLiteralType );
+
----------------
use type instead of auto


================
Comment at: clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.cpp:146
+    diag(MatchedInt->getBeginLoc(),
+         "non-portable integer literal: hardcoded platform-specific maximum value");
+  } else if (IsMin && !RepresentsZero) {
----------------
add here some hint to developer what to do, in cpp suggest using std::numeric_limits, in C some other MAX macro.
Same for other.


================
Comment at: clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.h:20
+/// http://clang.llvm.org/extra/clang-tidy/checks/portability/non-portable-integer-constant.html
+class NonPortableIntegerConstantCheck : public ClangTidyCheck {
+public:
----------------
this check is already in portability group, no need to duplicate "portable".
Maybe just portability-integer-constant


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/portability/non-portable-integer-constant.rst:22
+      return x ^ 0xFFFFFFFF;
+      // The right way to write this would be ULONG_MAX, or -1.
+    }
----------------
well, not, right way would be using std::numeric_limits<std::uint32_t>::max().
And unsigned long can be 4 bytes or 8 bytes or anything compiler decide.
So this example got more issues., and 0XFFF.. is a last of them, and -1 is not valid in such case.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/portability/non-portable-integer-constant.cpp:1
+// RUN: %check_clang_tidy %s -std=c++17-or-later portability-non-portable-integer-constant %t
+
----------------
no need to limit this check to c++17 or later.
when it's targeting all versions of C and C++


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/portability/non-portable-integer-constant.cpp:229
+  FULL_LITERAL;
+  // --CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-portable integer literal
+  // --CHECK-MESSAGES: :[[@LINE-3]]:22: note: expanded from macro
----------------
exclude system macros, otherwise it may warn for things like ULONG_MAX


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146712