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

2023-07-06 Thread Whisperity via Phabricator via cfe-commits 
whisperity requested changes to this revision.
whisperity added a comment.

Earlier (IIRC in March) we did an internal test of the check and the following 
results were obtained. The results are kinda //weird//, to say the least. 
Numbers are //after// CodeChecker has done a serverside uniqueing (most likely 
things such as //"if **`X`** in `H.h` is reported in the compilation/analysis 
of `A.cpp` and `B.cpp` then store **`X`** only once"//) of the bugs.

- memcache-d: 43
- tmux: 4
- curl: 283
- twin: 161
- vim: 2 206
- OpenSSL: **23 724**
- sqlite: 752
- ffmpeg: **87 121**
- postgres: **116 635**
- tinyxml2: 6
- libwebm: 34
- xerces: **48 732**
- bitcoin: 4 429
- protobuf: 1 405
- codechecker-ldlogger: 0
- ACID: 4 831
- Qt: **34 973**
- Contour v2: **14 246**
- LLVM: 6 018

No crashes (of the checker) were observed. Most of the large outlier numbers 
are attributable to heavy mathematics (ffmpeg, Qt), crypto (OpenSSL, 
PostgreSQL), or character encoding (Xerces, Postgres, Contour) operations in 
the project.

Unfortunately, this is a very significant amount of false positives to the 
point where the consumption of results is slowed down not on the mental load 
side, but also the technical side (such as triggering server hangs in 
CodeChecker and whatnot...). However, after putting some thoughts into it and 
trying to hand-clusterise some of the reports I've found a few significant 
opportunities where this check should be improved.

**Note:** These improvements might be worthwhile to implement as a //follow-up 
patch// which is merged together with this current review. **As it stands right 
now, this check is way too noisy (on maths-based projects) to be merged!**

1. Enums


One is when the literal is used to initialise an enum. So far I don't know 
whether simply just ignoring enums is good, or would it be worth to somehow 
emit a "unified" warning for the entire enum that follows the pattern as shown 
below.
F28149214: image.png 

2. `(u)?int([\d]+)_t`
-

The other are cases when the user explicitly initialises a **fixed-width** (or 
*constrained-width*) variable, such as `int64_t` or `uint16_t` with a literal. 
Right now, the implementation hard matches only the `IntegerLiteral` instances, 
i.e., lines such as `static constexpr std::uint64_t X = 0x07F00` where the 
width and characteristics of the literal might just as well *exactly* and 
*properly* match the variable it is assigned to (granted, we only match and 
silence `Decl`s for this, and lose expressivity on expressions), and do not 
report then.

3. Large arrays of magic literals
-

While this is not the silencing of a "valid" use case where the check, as of 
the current implementation, it is too verbose for, but I believe we could 
reasonably cull the noisiness of the check by simply saying that if there are 
array initialisers or //brace-init-lists// or whatever that contain multiple 
non-portable literals, then **either** we simply suck it up and do not warn for 
**any** of the elements... or for the sake of collegiality give **one** warning 
for the user. The former option would be the most silencing of all, while the 
latter would allow the user to do a single `// NOLINT` for the array instead of 
having to deal with `/* NOLINTBEGIN */ ... /* NOLINTEND */`.

Several chunks of noisy warnings come out of places like SHA-256, FFT, and 
various other "very mathematical" contexts where a large constant array is 
created with very specific proven values, all of which expressed and reported 
as "non-portable literals". It is no use to do 50 warnings for a single 
array. (This is perhaps after all the same(-ish) family as the "enum" problem.)




Comment at: 
clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.cpp:87
+  } else {
+return { StrippedLiteral, 0, 0, 10 };
+  }

(Unrealistic nit: This will consume hypothetical additional literals such as 
`0!ZZZbb=` as if they were decimal literals.)



Comment at: 
clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.cpp:153-154
+ "non-portable integer literal: integer literal with leading zeroes");
+  // Matches only the most significant bit,
+  // eg. unsigned value 0x8000.
+  } else if (IsMSBBitUsed) {

(Style nit: These lines could be joined and still fit 80-col, I think.)



Comment at: 
clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.h:16
+
+/// Finds integer literals that are being used in a non-portable manner.
+///

I am still not entirely convinced whether this one-line summary makes too much 
sense to a user who does not know the Standard out of heart... As we hunt for 
bit widths more or less perhaps we could weave this notion into the single 
sentence summary saying that integers with "non-portable

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

2023-03-23 Thread Piotr Zegar via Phabricator via cfe-commits 
PiotrZSL requested changes to this revision.
PiotrZSL added a comment.
This revision now requires changes to proceed.

Additionally, fix build.
Once you finish making changes to this review, switch it back to ready for 
review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146712

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-03-23 Thread 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 ^ 0x;
+  // The right way to write this would be ULONG_MAX, or -1.
+}

well, not, right way would be using std::numeric_limits::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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-03-23 Thread Eugene Zelenko via Phabricator via cfe-commits 
Eugene.Zelenko added a comment.

Please add alias entry in Release Notes.




Comment at: 
clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.cpp:95
+
+
+void NonPortableIntegerConstantCheck::check(

Excessive newline.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:136
 
+- New :doc:`portability-non-portable-integer-constant
+  ` check.

Incorrect entry.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/portability/non-portable-integer-constant.rst:4
+portability-non-portable-integer-constant
+==
+

Please make is same length as title.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146712

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-03-23 Thread Discookie via Phabricator via cfe-commits 
Discookie created this revision.
Discookie added reviewers: aaron.ballman, njames93, carlosgalvezp.
Discookie added a project: clang-tools-extra.
Herald added subscribers: PiotrZSL, ChuanqiXu, xazax.hun.
Herald added a project: All.
Discookie requested review of this revision.

This check finds integer literals that are being used in a non-portable manner.

Currently, the check detects cases where maximum or minimum values should be 
used
instead, as well as error-prone integer literals having leading zeroes, or
relying on most significant bits.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146712

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/portability/CMakeLists.txt
  clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.cpp
  clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.h
  clang-tools-extra/clang-tidy/portability/PortabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert/int17-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/portability/non-portable-integer-constant.rst
  
clang-tools-extra/test/clang-tidy/checkers/portability/non-portable-integer-constant.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/portability/non-portable-integer-constant.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/portability/non-portable-integer-constant.cpp
@@ -0,0 +1,258 @@
+// RUN: %check_clang_tidy %s -std=c++17-or-later portability-non-portable-integer-constant %t
+
+using int32_t = decltype(42);
+
+void regular() {
+  // no-warnings
+  0;
+  00;
+  0x0;
+  0x00;
+  0b0;
+  0b00;
+  0b0'0'0;
+
+  -1;
+  -0X1;
+
+  127;
+  0x7'f;
+
+  -128;
+  -0x80;
+
+  256;
+  0X100;
+
+  42;
+  0x2A;
+
+  180079837;
+  0xabbccdd;
+
+  // FIXME: (only 31 bits) False positive, reported as max signed int.
+  0b111;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-portable integer literal: hardcoded platform-specific maximum value [portability-non-portable-integer-constant]
+
+  // FIXME: Large numbers represented as hex are the most common false positive,
+  // eg. the following literal is a 64-bit prime.
+  0xff51afd7ed558ccd;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-portable integer literal: should not rely on the most significant bit [portability-non-portable-integer-constant]
+
+  // FIXME: Fixed-size integer literals are a common false positive as well.
+  int32_t literal = 0x4000;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: non-portable integer literal: should not rely on bits of most significant byte [portability-non-portable-integer-constant]
+
+  // FIXME: According to the standard, the type of the integer literal is the
+  // smallest type it can fit into. While technically a false positive, it could
+  // signal literal width confusion regardless.
+  long long int long_literal = 0x7fff;
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: non-portable integer literal: hardcoded platform-specific maximum value [portability-non-portable-integer-constant]
+}
+
+enum LiteralClamp {
+
+};
+
+// INT_MIN, INT_MAX, UINT_MAX, UINT_MAX-1
+// All binary literals are 32 bits long
+void limits_int() {
+  // FIXME: not recognize as Min
+  -214748'3'648;
+  // --CHECK-MESSAGES: :[[@LINE-1]]:4: warning: non-portable integer literal: hardcoded platform-specific minimum value [portability-non-portable-integer-constant]
+
+  -0x80'00'00'00;
+  // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: non-portable integer literal: hardcoded platform-specific minimum value [portability-non-portable-integer-constant]
+
+  -0200;
+  // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: non-portable integer literal: hardcoded platform-specific minimum value [portability-non-portable-integer-constant]
+
+  -0b1000;
+  // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: non-portable integer literal: hardcoded platform-specific minimum value [portability-non-portable-integer-constant]
+
+
+
+  21474'83647;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-portable integer literal: hardcoded platform-specific maximum value [portability-non-portable-integer-constant]
+
+  0x7FFF;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-portable integer literal: hardcoded platform-specific maximum value [portability-non-portable-integer-constant]
+
+  0177;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-portable integer literal: hardcoded platform-specific maximum value [portability-non-portable-integer-constant]
+
+  429'4'96'7295u;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: non-portable integer literal: hardcoded platform-specific maximum value [portability-non-portable-integer-constant]
+
+  0x;
+  //