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

[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] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Whisperity via Phabricator via cfe-commits]

whisperity added a subscriber: vabridgers.
whisperity added a comment.

@vabridgers Please take a look at this, as per off-list discussion, in relation 
to D142822 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140562

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

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:4627
   } else {
-Type *newType =
-  new (*this, TypeAlignment) InjectedClassNameType(Decl, TST);
+Type *newType = new (*this, TypeAlignment) InjectedClassNameType(Decl, 
TST);
 Decl->TypeForDecl = newType;

(Potentially unrelated change, only touching the format?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140562

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

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp:5-6
+
+// On arm and arch64 architectures, va_list is within the std namespace.
+// D136886 exposed an issue in the cert-dcl58-cpp checker where
+// implicit namespaces were not ignored.

(Style nits.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822

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

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp:3
+// RUN: %check_clang_tidy -std=c++17-or-later %s cert-dcl58-cpp %t -- -- -I 
%clang_tidy_headers -target aarch64
+// RUN: %check_clang_tidy -std=c++17-or-later %s cert-dcl58-cpp %t -- -- -I 
%clang_tidy_headers -target arm
+

balazske wrote:
> I am not sure if these platform specific should be added. A problem was 
> discovered on this platform but this could be true for all other tests.
I agree because there might be a case that some buildbot running the tests 
won't have these targets. If we really need to explicitly test something 
related to these targets, perhaps they should go into a separate file, and have 
a check-prefix. Or the expectation is that things work in the same way across 
all three platforms?



Comment at: clang/docs/ReleaseNotes.rst:63
   `Issue 60344 `_.
+- Fix importing of va_list types and declarations.
 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822

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

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Whisperity via Phabricator via cfe-commits]

whisperity added subscribers: rsmith, whisperity.
whisperity added a comment.

In D142822#4095896 , @DavidSpickett 
wrote:

>   
> /home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:14:18:
>  error: reference to 'std' is ambiguous
>   void f(str> ) {
>^
>   
> /home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:7:11:
>  note: candidate found by name lookup is 'std'
>   namespace std {
> ^

This looks like an ouchie of the highest order. I've been looking into 
Modules... 5-ish years ago, when it was still under design. There are two major 
concerns here, one is that we've been having a feature called //"Modules"// 
which was very Clang-specific (but a good proof-of-concept spiritual 
predecessor of what became the Standard Modules), and there are the standard 
modules. Now the file name explicitly mentions `cxx20` so this is likely about 
Standard Modules stuff. But to be fair, I'd take this implementation with a 
pinch of salt. Due to the lack of build-system support for Modules (there are 
some murmurs these days on the CMake GitLab about very soon adding meaningful 
support for this...) we do not really seem to have large-scale real-world 
experience as to how the standard really could be made working, **especially** 
in such a weird case like touching `namespace std` which //might// not be 
standards-compliant in the first place. (But yeah, we are test code here in a 
compiler; we can cut ourselves some slack.)

The code itself in these test files is really weird; it seems the test is 
centred around the idea that there are:

  // TU "A"
  module; // Code in the *global module fragment*
  namespace std { /* ... */ class basic_string; }
  
  export module Whatever; // Code in the public module interface unit of module 
Whatever.
  export /* ... */ using str = std::basic_string<...>;

So we have a symbol called `str`, which is publicly visible (to TUs importing 
`Whatever`), and the `namespace std` preceding `Whatever` in `TU "A"` is 
**not** visible but **reachable**? (The standard's way of expressing 
reachability is a kind of a mess , so I'd 
**definitely** try finding the people who worked in developing the actual 
solution that makes Standard Modules //work// in Clang... call the help of 
someone who's really expert on the Standard...) Perhaps @rsmith could help us 
clarify this situation.

You can't name `std::basic_string<...>` in the client code (because it is not 
//visible//), but you can depend on the symbol (e.g., `decltype` it to an 
alias!) because it is //reachable//!

And then you have

  // TU "B"
  import Whatever;
  
  namespace std { /* ... */ struct char_traits {}; }
  
  // use Sb mangling, not Ss, as this is not global-module std::char_traits
  /* ... */
  void f(str<..., std::char_traits<...>>)

In which there is "another" (?) `namespace std`, which should(??) also be part 
of the global module :

> The //global module// is the collection of all //global-module-fragments// 
> and all translation units that are not module units. Declarations appearing 
> in such a context are said to be in the //purview// of the global module.

I believe that `TU "B"` is **not** a module unit... But the comment (which is 
somehow misformatted?) in it would like to say that this `std` is, in fact, 
//not// part of the global module.

So somehow, the changes in this patch are making //both// `std`s part of the 
same //purview// (whatever that means, really...), thus resulting in ambiguity.

- Does this only happen on non-X86? @vabridgers, you could try adding a 
`--target=` flag locally in the test file to the compiler invocation and 
re-running the tests to verify. (Might need an LLVM build that is capable of 
generating code of other architectures.)
- My other suggestion would be putting some `->dump()` calls in your change, 
running the code and observing how the ASTs are looking in the states when 
execution is within the functions you're trying to change.




Comment at: clang/test/AST/ast-dump-traits.cpp:55
 // CHECK-NEXT: | `-ExpressionTraitExpr {{.*}}  'bool' 
__is_lvalue_expr
-// CHECK-NEXT: `-FunctionDecl {{.*}}  line:30:6{{( 
imported)?}} test_unary_expr_or_type_trait 'void ()'
+// CHECK-NEXT:  -FunctionDecl {{.*}}  line:30:6{{( 
imported)?}} test_unary_expr_or_type_trait 'void ()'
 // CHECK-NEXT:   `-CompoundStmt {{.*}} 

How is this change legit? Is this `FunctionDecl` node just "floating"? If so, 
why is there a `-` preceding in the textual dump? Usually top-level nodes do 
not have such a prefix.



Comment at: clang/test/AST/fixed_point.c:405
 
-//CHECK-NEXT: `-VarDecl {{.*}} literallast '_Accum' cinit
+//CHECK-NEXT:  -VarDecl {{.*}} literallast '_Accum' cinit
 //CHECK-NEXT:   `-FixedPointLiteral {{.*}} '_Accum' 1.0

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D91000#4099773 , @dyung wrote:

> To XFAIL the test, grep for lines with "XFAIL" and "ps4" and you should find 
> some examples. It was recently changed how it worked lately.

Thank you, yes I found an example. I went with `UNSUPPORTED` instead of `XFAIL` 
because the test implements 4 or 5 separate cases and `XFAIL`ing it all would 
make some cases become //"Unexpectedly passed"//... I did a quick commit with 
marking it as unsupported with a bit of an explanation in the code, and now the 
buildbots are coming back green.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D91000#4099669 , @dyung wrote:

>   -DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-scei-ps4
>
> Hopefully this can help you to reproduce the problem.

Ah, thank you very much. And indeed, giving `--target=x86_64-scei-ps4` to 
Clang-Tidy will make the test case fail, but using `x86_64-linux-gnu` works 
perfectly.
Now knowing the exact triple I think I can delve into the preprocessor stack or 
something to figure out why the //Annex K.//-related flags, my best bet is that 
it's explicitly forbidden from being defined. 

  clang-tidy 
/tmp/LLVM-Build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/unsafe-functions.c.tmp.c
 --checks='-*,bugprone-unsafe-functions' -config={} -- 
--target=x86_64-linux-gnu -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1 
-nostdinc++

My first wish is to figure out how to reliably disable the test case and have 
this quick-fix pushed immediately so the buildbots don't continue failing, and 
then we can figure out that this check should be disabled on this specific 
platform, or something like that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a subscriber: dyung.
whisperity added a comment.

Ping @dyung, it looks like you're the owner of the relevant build-bot. I can't 
find any information what `x86_64-sie` is...

Meanwhile, it seems my attempt at fix is not actually fixing anything, I'm 
trying to figure out how to at least disable the check on this specific 
architecture. The rest of the architectures seem to be passing normally as 
expected. Something must be special within Clang's default environment or 
compiler configuration...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Alright, right now, I have no meaningful idea why this failure appears 
,
 locally I'm trying every test command as they appear in the test, and all the 
tests are passing. It's as if on that system the whole //Annex K// support 
would not be allowed. Locally I added a few debug prints and I'm getting the 
right answers for //"Whether Annex K is allowed?"//.

@njames93 @aaron.ballman Do you have any idea what could make that test fail in 
that system?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Oddly enough, one of the buildbots caught a not matching test that was working 
locally... on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf27c8ac83e7c: [clang-tidy] Add the 
`bugprone-unsafe-functions` check (authored by futogergely, committed by 
whisperity).

Changed prior to commit:
  https://reviews.llvm.org/D91000?vs=485774=494265#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert/msc24-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert/msc33-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c
@@ -0,0 +1,154 @@
+// RUN: %check_clang_tidy -check-suffix=WITH-ANNEX-K%s bugprone-unsafe-functions %t -- -- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1
+// RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s bugprone-unsafe-functions %t -- -- -U__STDC_LIB_EXT1__   -U__STDC_WANT_LIB_EXT1__
+// RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s bugprone-unsafe-functions %t -- -- -D__STDC_LIB_EXT1__=1 -U__STDC_WANT_LIB_EXT1__
+// RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s bugprone-unsafe-functions %t -- -- -U__STDC_LIB_EXT1__   -D__STDC_WANT_LIB_EXT1__=1
+// RUN: %check_clang_tidy -check-suffix=WITH-ANNEX-K-CERT-ONLY  %s bugprone-unsafe-functions %t -- \
+// RUN:   -config="{CheckOptions: [{key: bugprone-unsafe-functions.ReportMoreUnsafeFunctions, value: false}]}" \
+// RUN:-- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1
+
+typedef __SIZE_TYPE__ size_t;
+typedef __WCHAR_TYPE__ wchar_t;
+
+char *gets(char *S);
+size_t strlen(const char *S);
+size_t wcslen(const wchar_t *S);
+
+void f1(char *S) {
+  gets(S);
+  // CHECK-MESSAGES-WITH-ANNEX-K:   :[[@LINE-1]]:3: warning: function 'gets' is insecure, was deprecated and removed in C11 and C++14; 'gets_s' should be used instead
+  // CHECK-MESSAGES-WITH-ANNEX-K-CERT-ONLY: :[[@LINE-2]]:3: warning: function 'gets' is insecure, was deprecated and removed in C11 and C++14; 'gets_s' should be used instea
+  // CHECK-MESSAGES-WITHOUT-ANNEX-K::[[@LINE-3]]:3: warning: function 'gets' is insecure, was deprecated and removed in C11 and C++14; 'fgets' should be used instead
+
+  strlen(S);
+  // CHECK-MESSAGES-WITH-ANNEX-K:   :[[@LINE-1]]:3: warning: function 'strlen' is not bounds-checking; 'strnlen_s' should be used instead
+  // CHECK-MESSAGES-WITH-ANNEX-K-CERT-ONLY: :[[@LINE-2]]:3: warning: function 'strlen' is not bounds-checking; 'strnlen_s' should be used instead
+  // no-warning WITHOUT-ANNEX-K
+}
+
+void f1w(wchar_t *S) {
+  wcslen(S);
+  // CHECK-MESSAGES-WITH-ANNEX-K:   :[[@LINE-1]]:3: warning: function 'wcslen' is not bounds-checking; 'wcsnlen_s' should be used instead
+  // CHECK-MESSAGES-WITH-ANNEX-K-CERT-ONLY: :[[@LINE-2]]:3: warning: function 'wcslen' is not bounds-checking; 'wcsnlen_s' should be used instead
+  // no-warning WITHOUT-ANNEX-K
+}
+
+struct tm;
+char *asctime(const struct tm *TimePtr);
+
+void f2(const struct tm *Time) {
+  asctime(Time);
+  // CHECK-MESSAGES-WITH-ANNEX-K:   :[[@LINE-1]]:3: warning: function 'asctime' is not bounds-checking and non-reentrant; 'asctime_s' should be used instead
+  // CHECK-MESSAGES-WITH-ANNEX-K-CERT-ONLY: :[[@LINE-2]]:3: warning: function 'asctime' is not bounds-checking and non-reentrant; 'asctime_s' should be used instead
+  // CHECK-MESSAGES-WITHOUT-ANNEX-K::[[@LINE-3]]:3: warning: function 'asctime' is not bounds-checking and non-reentrant; 'strftime' should be used instead
+
+  char *(*F1)(const struct tm *) = asctime;
+  // CHECK-MESSAGES-WITH-ANNEX-K:   :[[@LINE-1]]:36: warning: function 'asctime' is not bounds-checking and non-reentrant; 'asctime_s' should be used instead
+  // CHECK-MESSAGES-WITH-ANNEX-K-CERT-ONLY: :[[@LINE-2]]:36: warning: function 'asctime' is not bounds-checking and non-reentrant; 'asctime_s' should be used instead
+  // CHECK-MESSAGES-WITHOUT-ANNEX-K::[[@LINE-3]]:36: warning: function 'asctime' is not bounds-checking and non-reentrant; 'strftime' should be used instead
+
+  char *(*F2)(const struct tm *) = 
+  // CHECK-MESSAGES-WITH-ANNEX-K:   

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity accepted this revision.
whisperity added a comment.
This revision is now accepted and ready to land.

Alright, I've done a full reanalysis after a rebase. Overhead is not 
meaningfully measurable, even for complex TUs such as LLVM. The check is viable 
in C++ code as it finds cases (such as the one described in LLVM, but also 
Bitcoin is a primarily C++ project), so I won't rework the check to disable it 
in C++ mode explicitly. It seems the name lookup is implemented pretty well, 
helped by the fact that the names to match are short. No crashes had been 
observed in the test projects; let's hope it stays the same way; the matchers 
themselves are simple enough. The //Annex K.// matcher is only registered if in 
>= C11 mode.
I've also gone through the discussion earlier, and it looks to me as if all the 
concerns were either resolved or made obsolete due to the evolution of the 
check.

I did make several **NFC** changes to the patch in post-production, such as 
fixing the documentation here and there, ensuring that the `cert-` aliases are 
appropriately documented, and a little bit of refactoring so internal details 
of the check are genuinely internal. Thus, I'll be committing a version that is 
different from the one here.


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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Alright, the tests came back from the CI positively and there were no crashes 
on like 15 projects (about 50:50 C and C++). There are some nice results:

- In LLVM, in `LexPPMacroExpansion.cpp` `ExpandBuiltinMacro()` there is a hit 
for a call `Result = asctime(TM);`. The suggestion is `strftime` which is 
available in all C++ versions and >= C99, so this seems like a valid finding. 
`Result` is a `const char *` which is initialised by this call.
- In Bitcoin, **2** findings in `logging.cpp` doing `setbuf(out, nullptr);`. 
Not sure if these are legit findings because these seem to be only "resetting" 
some buffering, but the warning message is legible.
- In Postgres:
  - **2** findings in `isolationtester.c`, same `setbuf(stdout, NULL);` and 
`setbuf(stderr, NULL);`. `setvbuf/4` is available from C99 so this seems like a 
valid finding from a language version standpoint.
  - In `initdb.c`, a call to `rewind` with the comment preceding: //`/* now 
reprocess the file and store the lines */`//.
- In SQLite:
  - In `lemon.c`, a call to `rewind` [...]
  - In `shell.c`, **3** calls to `rewind` [...]
- In OpenSSL:
  - In `randfile.c` and `apps.c`, **2** `setbuf(fileptr, NULL);` calls [...]
- In Vim, **4** calls to `rewind()` in `xxd.c`, `tag.c`, and `term.c`.
  - The case in `term.c` is extra funny because directly following the 
`rewind(fd)` call there is a `while (!feof(fd)) ...`
- In MemcacheD, a `setbuf(stderr, NULL);` with the comment //`/* set stderr 
non-buffering ... */`//

It seems not many projects use Annex K at least from this test set. Note that 
we're primarily working on Linux, where Annex K isn't in the standard library. 
It has also been reported to not that useful 
, although it's 
good that the check uses them optionally. In general, we seem to have made a 
good enough framework for registering "unsafe standard APIs" later on with 
those stringswitches.

Regarding the performance, I did not see any meaningful slow-down. Most of the 
projects analysed in a few seconds or minutes, similar to other Tidy runs in 
the same infrastructure.


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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Tentative **LGTM**, will still need to run the "usual test projects" in the CI 
for confirmation. What's missing from this patch are the `.rst` files for the 
CERT-specific aliases of the check, but those are trivial and we can "add them 
in post" anyway.


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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D91000#4020891 , @futogergely wrote:

> What we could do is:
>
> 1. add a new checker option to decide if we suggest replacements from AnnexK. 
> We could avoid registering matchers this way, but I don't really like this, 
> having an option for something we could decide from the defined macros.
> 2. As a TODO, we could make possible to register checkers AFTER the 
> preprocessor is executed. I have not looked into this, so I don't really know 
> if it is possible at all in the current architecture.

I think it's fine if we do not go a great length for changing the entire 
infrastructure around this. At least we looked, it's not possible.

@aaron.ballman said that we've likely did as much as we could. At least the 
matchers themselves aren't that expensive, it's a small string lookup 
(hopefully hashed or at least memoised) in a trivial search for a very specific 
node type only.

I'll try running at least one final grand CI test of this check just to make 
sure it's not crashing and such, but due to the C11 **and** //Annex K// 
requirement, I do not expect a lot of results...


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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D91000#3861942 , @aaron.ballman 
wrote:

> My concern with that approach was that we pay the full expense of doing the 
> matches only get get into the `check()` function to bail out on all the Annex 
> K functions, but now that there are replacements outside of Annex K, I don't 
> see a way around paying that expense, so I think my concern has been 
> addressed as well as it could have been.

I think that Clang-Tidy checks are instantiated per AST. I will look into 
whether we can somehow do the disabling of the check as early as possible! (In 
that case, we could simply NOT register the matcher related to Annex-K 
functions.) Either way, I'll do a rebase, re-run the tests and etc., and likely 
take over the check.


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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D91000#3770071 , @whisperity wrote:

> In D91000#3197851 , @aaron.ballman 
> wrote:
>
>> In terms of whether we should expose the check in C++: I think we shouldn't. 
>> [...]
>>
>> I think we should probably also not enable the check when the user compiles 
>> in C99 or earlier mode, because there is no Annex K available to provide 
>> replacement functions.
>
> @aaron.ballman I think the current version of the check satisfies these 
> conditions. It seems the check **will** run for every TU, but in case there 
> is no alternative the check could suggest, it will do nothing:
>
>   if (!ReplacementFunctionName)
> return;
>
> Is this good enough? This seems more future-proof than juggling the 
> `LangOpts` instance in yet another member function.

@aaron.ballman @njames93 Ping!
It seems @futogergely has resigned from the company, so I'll end up flying the 
approach, but the one above is the last outstanding question.


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

https://reviews.llvm.org/D91000

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

[PATCH] D133119: [clang-tidy] Add checker 'bugprone-suspicious-realloc-usage'.

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp:89-90
+return;
+  diag(Call->getBeginLoc(),
+   "memory block passed to 'realloc' may leak if 'realloc' fails");
+}

whisperity wrote:
> I have some reservations about this message, though. It only indirectly 
> states the problem: first, the user needs to know what a memory leak is, and 
> then needs to know how realloc could fail, and then make the mental 
> connection about the overwriting of the pointer. (Also, you're passing a 
> pointer... "memory block" is a more abstract term, requiring more knowledge.)
> 
> I think for the sake of clarity, we should be more explicit about this.
> 
> > taking the returned pointer from 'realloc' overwrites 'XXXPtr' with null
> > if allocation fails,
> > which may result in a leak of the original buffer
> 
> 1st line explains how the issue manifests (explicitly saying that you'll get 
> a nullpointer in a bad place)
> 2nd line gives the condition, which is a bit more explicit
> 3rd line explains the real underlying issue
> 
> (Getting the null pointer is the "only" bad path here.)
Also:

```lang=cpp
diag(...) << PtrInputExpr.getSourceRange() << PtrResultExpr.getSourceRange();
```

So the connection that the recipient of the assignment and the 1st parameter is 
the same is more obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133119

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

[PATCH] D133119: [clang-tidy] Add checker 'bugprone-suspicious-realloc-usage'.

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp:89-90
+return;
+  diag(Call->getBeginLoc(),
+   "memory block passed to 'realloc' may leak if 'realloc' fails");
+}

I have some reservations about this message, though. It only indirectly states 
the problem: first, the user needs to know what a memory leak is, and then 
needs to know how realloc could fail, and then make the mental connection about 
the overwriting of the pointer. (Also, you're passing a pointer... "memory 
block" is a more abstract term, requiring more knowledge.)

I think for the sake of clarity, we should be more explicit about this.

> taking the returned pointer from 'realloc' overwrites 'XXXPtr' with null
> if allocation fails,
> which may result in a leak of the original buffer

1st line explains how the issue manifests (explicitly saying that you'll get a 
nullpointer in a bad place)
2nd line gives the condition, which is a bit more explicit
3rd line explains the real underlying issue

(Getting the null pointer is the "only" bad path here.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133119

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

[PATCH] D133119: [clang-tidy] Add checker 'bugprone-suspicious-realloc-usage'.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

I agree with @martong that `$ = realloc($, ...)` is indicative of something 
going wrong, even if the developer has saved the original value somewhere. Are 
we trying to catch this with Tidy specifically for this reason (instead of 
CSA's stdlib checker, or some taint-related mechanism?). However, @steakhal has 
some merit in saying that developers might prioritise the original over the 
copy, even if they made a copy. I think an easy solution from a documentation 
perspective is to have a test for this at the bottom of the test file. And 
document that we cannot (for one reason or the other) catch this supposed 
solution, **but** //if// you have made a copy already(!), then you might as 
well could have done `void* q = realloc(p, ...)` anyway! Having this documented 
leaves at least //some// paths open for us to fix false positives later, if 
they become a tremendous problem. (I have a gut feeling that they will not, 
that much.)




Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp:20-21
+namespace {
+class IsSamePtrExpr : public clang::StmtVisitor /*,
+ public clang::DeclVisitor*/
+{

steakhal wrote:
> Commented code?
(Nit: `using namespace clang` -> The `clang::` should be redundant here.)



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp:87
+return;
+  if (!IsSamePtrExpr().check(PtrInputExpr, PtrResultExpr))
+return;

(We're creating a temporary here, right?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133119

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Generally LGTM. Please revisit the documentation and let's fix the style, and 
then we can land.

In D91000#3197851 , @aaron.ballman 
wrote:

> In terms of whether we should expose the check in C++: I think we shouldn't. 
> [...]
>
> I think we should probably also not enable the check when the user compiles 
> in C99 or earlier mode, because there is no Annex K available to provide 
> replacement functions.

@aaron.ballman I think the current version of the check satisfies these 
conditions. It seems the check **will** run for every TU, but in case there is 
no alternative the check could suggest, it will do nothing:

  if (!ReplacementFunctionName)
return;

Is this good enough? This seems more future-proof than juggling the `LangOpts` 
instance in yet another member function.




Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp:1
+//===--- UnsafeFunctionsCheck.cpp - clang-tidy ---===//
+//

Ditto.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp:150
+   Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) 
{
+  this->PP = PP;

(Just so that we do not get "unused variable" warnings when compiling.)



Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h:1
+//===--- UnsafeFunctionsCheck.h - clang-tidy ---*- C++ -*-===//
+//

Style nit: length of line is not padded to 80-col.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst:31-40
+The following functions are also checked (regardless of Annex K availability):
+ - `rewind`, suggested replacement: `fseek`
+ - `setbuf`, suggested replacement: `setvbuf`
+
+Based on the ReportMoreUnsafeFunctions option, the following functions are 
also checked:
+ - `bcmp`, suggested replacement: `memcmp`
+ - `bcopy`, suggested replacement: `memcpy_s` if Annex K is available, or 
`memcopy` if Annex K is not available

In general, the **rendered version** of the docs should be **visually** 
observed, as backtick in RST only turns on emphasis mode and not monospacing, 
i.e. it will be //memcpy_s// and not `memcpy_s`. Please look at the other 
checks and if there is a consistent style (e.g. symbol names (function) are 
monospaced, but options of the check is emphasised) align it to that so we have 
the documentation "fall into place" visually.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst:52
+Both macros have to be defined to suggest replacement functions from Annex K. 
`__STDC_LIB_EXT1__` is
+defined by the library implementation, and `__STDC_WANT_LIB_EXT1__` must be 
defined to "1" by the user
+before including any system headers.

(Style nit: yeah this will not look like a symbol at all here)


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

https://reviews.llvm.org/D91000

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

[PATCH] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

[Whisperity via Phabricator via cfe-commits]

whisperity removed a reviewer: bzcheeseman.
whisperity added a comment.
This revision now requires review to proceed.

In D131319#3708667 , @bzcheeseman 
wrote:

> This is great, thank you for doing this! I'm not a competent reviewer for the 
> actual clang-tidy code changes but the +1 for the idea :)

The problem with the approval here is that a single approval will turn the 
patch into a fully approved state, which breaks the dashboards for people added 
to the patch (i.e., other reviewers will think the patch is already approved, 
and thus perhaps will not consider putting effort into reviewing it!).

However, I think you should try the //Award Token// option from the menu on the 
right! Somewhere the awarded tokens should show up on the patch, tallying up 
support!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131319

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

[PATCH] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp:12-13
 
 template 
 bool isa(Y *);
 template 

Will the tests pass properly once the fixes are applied, even though the 
replaced code refers to a symbol (`isa_and_present`) that is not declared in 
the TU?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131319

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

[PATCH] D118996: [clang-tidy] Support C++14 in bugprone-signal-handler.

[Whisperity via Phabricator via cfe-commits]

whisperity accepted this revision.
whisperity added a comment.

Alright, I think we should have this in and let C++17 things to be future work. 
Please see the inline comment, but otherwise this should be good enough. Can 
always improve in a future version. 

LLVM 15 has branched off, so this should be rebased for the last time against 
the current main branch, just to ensure everything is good.




Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:284-298
+bool isCXXOnlyStmt(const Stmt *S) {
+  StringRef Name = S->getStmtClassName();
+  if (Name.startswith("CXX"))
+return true;
+  // Check for all other class names in ExprCXX.h that have no 'CXX' prefix.
+  return isahttps://reviews.llvm.org/D118996/new/

https://reviews.llvm.org/D118996

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

[PATCH] D124447: [clang-tidy] Add infrastructure support for running on project-level information

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D124447#3651074 , @whisperity 
wrote:

> I will attempt to write a concise response to this today (and tomorrow)! 
> Sorry, I was away travelling to and doing post-action bureaucracy of 
> conferences the past few weeks.

@aaron.ballman @kadircet @sammccall, FYI, I have posted it here: 
http://discourse.llvm.org/t/rfc-enhancing-clang-tidy-with-project-level-knowledge/63960


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124447

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

[PATCH] D124447: [clang-tidy] Add infrastructure support for running on project-level information

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D124447#3645197 , @sammccall wrote:

> In D124447#3608747 , @aaron.ballman 
> wrote:
>
>> In general, I think this is looking pretty close to good. Whether clang-tidy 
>> should get this functionality in this form or not is a bit less clear to me. 
>> *I* think it's helpful functionality and the current approach is reasonable, 
>> but I think it might be worthwhile to have a community RFC to see if others 
>> agree. CC @alexfh in case he wants to make a code owner decision instead.
>
> +1 to a need for the RFC here, there are ecosystem questions that I think 
> should be addressed first but aren't in scope of this patch.

I will attempt to write a concise response to this today (and tomorrow)! Sorry, 
I was away travelling to and doing post-action bureaucracy of conferences the 
past few weeks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124447

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

[PATCH] D112916: [clang-tidy] Confusable identifiers detection

[Whisperity via Phabricator via cfe-commits]

whisperity accepted this revision.
whisperity added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


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

https://reviews.llvm.org/D112916

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

[PATCH] D126891: [clang-tidy] The check should ignore final classes

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

(Please ensure a more appropriate commit message that actually mentions the 
check when committing, @steakhal.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126891

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

[PATCH] D112916: [clang-tidy] Confusable identifiers detection

[Whisperity via Phabricator via cfe-commits]

whisperity requested changes to this revision.
whisperity added a comment.
This revision now requires changes to proceed.

Because of the stability issues related to `getName()`-like constructs I'm 
putting a temporary ❌ on this (so it doesn't show up as faux accept). However, 
I have to emphasise that I do like the idea of the check!




Comment at: clang-tools-extra/clang-tidy/misc/CMakeLists.txt:50
   omp_gen
+  genconfusable
   )

`gen_confusable_glyph_list`?



Comment at: 
clang-tools-extra/clang-tidy/misc/ConfusableTable/build_confusable_table.cpp:1
+//===--- build_confusable_table.cpp - 
clang-tidy---===//
+//

Why does this file have `snake_case` name?


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

https://reviews.llvm.org/D112916

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

[PATCH] D112916: [clang-tidy] Confusable identifiers detection

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/Homoglyph.cpp:32-35
+/**
+ * Build a skeleton out of the Original identifier, following the algorithm
+ * described in http://www.unicode.org/reports/tr39/#def-skeleton
+ */

`/*` -> `//` or `///` (latter is for documentation comments)



Comment at: clang-tools-extra/clang-tidy/misc/Homoglyph.cpp:41-42
+
+  char const *Curr = SName.c_str();
+  char const *End = Curr + SName.size();
+  while (Curr < End) {

(Nit: in LLVM we use //"const west"//)



Comment at: clang-tools-extra/clang-tidy/misc/Homoglyph.cpp:48
+llvm::ConversionResult Result = llvm::convertUTF8Sequence(
+(const llvm::UTF8 **), (const llvm::UTF8 *)End, ,
+llvm::strictConversion);

These casts are weird when reading the code. The problem with C-style casts is 
that the compiler will try to resolve it to //some// kind of cast, which makes 
them less "explicit" than would be good for type safety. Could you replace 
these with `static_cast`s? Or are these, in fact, `reinterpret_cast`s?



Comment at: clang-tools-extra/clang-tidy/misc/Homoglyph.cpp:64-68
+  llvm::UTF8 Buffer[32];
+  llvm::UTF8 *BufferStart = std::begin(Buffer);
+  llvm::UTF8 *IBuffer = BufferStart;
+  const llvm::UTF32 *ValuesStart = std::begin(Where->values);
+  const llvm::UTF32 *ValuesEnd =

(Nit: consider adding a `using namespace llvm;` **inside** the function and 
then you could reduce the length of these types.)



Comment at: clang-tools-extra/clang-tidy/misc/Homoglyph.cpp:84
+  if (const auto *ND = Result.Nodes.getNodeAs("nameddecl")) {
+if(  IdentifierInfo *   II = ND->getIdentifier()) {
+  StringRef NDName = II->getName();

(Nit: formatting is broken here.)



Comment at: clang-tools-extra/clang-tidy/misc/Homoglyph.cpp:86-88
+  auto  = Mapper[skeleton(NDName)];
+  auto *NDDecl = ND->getDeclContext();
+  for (auto *OND : Mapped) {

(Nit: we tend to use `auto` if and only if the type of the variable is either 
reasonably impossible to spell out (e.g. an iterator into a container with 
3784723 parameters) or immediately obvious from the initialiser expression 
(e.g. the init is a cast).)



Comment at: clang-tools-extra/clang-tidy/misc/Homoglyph.cpp:93
+if (OND->getName() != NDName) {
+  diag(OND->getLocation(), "%0 is confusable with %1")
+  << OND->getName() << NDName;

Perhaps you could elaborate on "confusable" here a bit for the user?



Comment at: clang-tools-extra/clang-tidy/misc/Homoglyph.cpp:94
+  diag(OND->getLocation(), "%0 is confusable with %1")
+  << OND->getName() << NDName;
+  diag(ND->getLocation(), "other definition found here",

Careful: `getName()` might assert away if the identifier isn't "trivially 
nameable", e.g. `operator==`. Is this prevented somewhere? (Otherwise, could we 
add a negative test case just to ensure no crashes start happening?) Operators 
are also `NamedDecl`s.



Comment at: clang-tools-extra/clang-tidy/misc/Homoglyph.cpp:95
+  << OND->getName() << NDName;
+  diag(ND->getLocation(), "other definition found here",
+   DiagnosticIDs::Note);

Definition or declaration? "Entity"?



Comment at: clang-tools-extra/clang-tidy/misc/Homoglyph.h:18
+
+class Homoglyph : public ClangTidyCheck {
+public:

This is the "top-level" check that is the addition in this patch, right?

In that case, this class is breaking naming conventions (usually the symbol 
name ends with `Check`) and the "usual" documentation comment from the class is 
also missing.

-

Also, maybe a full rename to `ConfusableIdentifierCheck` would be worth it. If 
you'd like to stick to the current summary of the patch.



Comment at: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp:41
 "misc-misleading-bidirectional");
+CheckFactories.registerCheck("misc-homoglyph");
 CheckFactories.registerCheck(

The comment about coming up with a better name for the check I think applies 
here too. I would be comfortable with `misc-confusable-identifiers`. The 
problem with `homoglyph` is that it requires a specific understanding of 
English and (natural) language studies that we should not hard-require from our 
users. If I have to look up on Wikipedia what the name of the rule I'm using 
means then something is wrong.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:141
+
+  Detects confusable unicode identifiers.
+

**U**nicode? Isn't it a trademark?



Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-homoglyph.rst:8
+each other, 

[PATCH] D126247: [clang-tidy][doc] Document readability-indentifier-naming resolution order and examples

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2750-2752
+where an individual identifier can fall into several classifications. Below
+is a list of the classifications supported by ``readability-identifier-naming``
+presented in the order in which the classifications are resolved. Some

So is this resolution order the same for //every Decl//?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2880
+
+The code snippet below[1]_ serves as an exhaustive example of various
+identifiers the ``readability-identifier-naming`` check is able to classify.

LegalizeAdulthood wrote:
> Should we be linking to ephemeral gists that can disappear?
Are the contents of the file linked and the one in the documentation here the 
exact same? In that case I think if the author gives rights for LLVM to use 
their work, we can get rid of the link, as it serves no additional value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126247

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

[PATCH] D126891: [clang-tidy] The check should ignore final classes

[Whisperity via Phabricator via cfe-commits]

whisperity accepted this revision.
whisperity added a comment.
This revision is now accepted and ready to land.

Thank you!




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:170
+  ` involving
+  ``final`` classes. The check will not diagnose ``final`` marked classes, 
since
+  those cannot be used as base classes, consequently they can not violate the

(Nit: classes marked final)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126891

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

[PATCH] D127599: [clang] small speed improvement of Sema::AddArgumentDependentLookupCandidates

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

The context of the diff is missing. Please re-run the diff making with 
`-U999`.




Comment at: clang/include/clang/Sema/Lookup.h:817
+  bool empty(void) { return Decls.empty(); }
+
   using iterator =

`(void)`? LLVM is a C++ project.


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

https://reviews.llvm.org/D127599

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

[PATCH] D124446: [clang-tidy] Add the misc-discarded-return-value check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D124446#3521619 , @whisperity 
wrote:

> @aaron.ballman [...] I think I can put in a measurement job [...]

I've rerun the experiment **on another computer** (compared to the results 
shown in the inline comments) from scratch. The measurement environment had a 
4core/8thread CPU with 16 GiB RAM. There were no OOM situations.
LLVM was self-built by me, in a //Release// build (with 
`-DBUILD_SHARED_LIBS=ON`), linked with `ld.gold`.

The analysis was executed with the //"core checks"// list consisting of **all** 
`bugprone` (excluding bugprone-unchecked-optional-access 
), `misc`, `modernize`, `performance`, `portability`, 
and `readability`; with `-wall -wextra -weverything` and the //Clang Static 
Analyser// turned **off**. (This means that purely guidelines-related checks 
were also not run.)

- //BURV//: `bugprone-unused-return-value`
- //MDRV//: `misc-discarded-return-value`

Below times are the **full** analysis, for the project, at `-j8`, including the 
parsing and whatnot. Times are `h:mm:ss.fff`, real-world elapsed (//"wall"//) 
time.
I've also utilised Tidy's `--export-check-profile` feature. I've run the 
analysis results through CSA-Testbench 
 to generate some graphs, 
attached for each project. I know they look ugly, but I tried to make the 
colours as consistent between projects and executions as possible. (The two 
highlighted checks, //BURV// and //MDRV// are split out of the chart and get 
consistent bright colours. The rest of the colours for every check is 
calculated from the hashing of the check's full name, and as such, the results 
should be comparable even across projects.) The input for each entry in the 
chart is the summed up (for the entire project) `.wall` time, per check, as 
reported by Tidy in the JSONs it emitted.

| **Project**   
  | **Core checks** | Core //+ BURV// | Core //+ MDRV// 
| Core + **//BURV//, //MDRV//** | **only** //BURV// | **only** //MDRV// | 
**only** //BURV//, //MDRV// | **Chart** |
| Bitcoin   
  | 0:13:52.088 | 0:14:16.517 | 0:14:54.725 
| 0:15:18.858   | 0:01:35.215   | 0:02:15.282   | - 
  | F23156342: bitcoin.png 
 |
| CodeChecker ld-logger 

 | 0:00:01.055 | 0:00:01.033 | 0:00:01.098 | 0:00:01.132
   | 0:00:00.334   | 0:00:00.401   | -   | 
F23156348: codechecker.png  |
| Contour Terminal Emulator 
  | 
0:12:54.247 | 0:13:10.559 | 0:13:23.222 | 0:13:49.406   
| 0:01:27.554   | 0:01:57.877   | -   | 
F23156351: contour.png  |
| cURL    
  | 0:01:47.186 | 0:01:50.722 | 0:01:48.436 
| 0:01:51.260   | 0:00:16.982   | 0:00:22.576   | - 
  | F23156353: curl.png 
 |
| FFmpeg   
  | 0:08:45.986 | 0:09:09.407 | 0:09:20.924 
| 0:09:20.310   | 0:01:03.081   | 0:01:28.070   | - 
  | F23156355: ffmpeg.png 
 |
| libWebm  
  | 0:00:12.737 | 0:00:13.759 | 0:00:13.775 
| 0:00:14.609   | 0:00:01.220   | 0:00:01.872   | - 
  | F23156357: libwebm.png 
 |
| LLVM (Clang + CTE)   
  |  DNF   | -   | -   
| - | 0:59:29.017   | 1:21:39.916   | 
1:30:37.199 | F23156359: llvm-project.png 
 |
| MemcacheD   
  | 0:00:07.288 | 0:00:09.124 | 0:00:07.981 
| 0:00:11.056   | 0:00:01.561   | 0:00:01.938   | - 
  | F23156361: memcached.png 
 |
| MongoDB  
 

[PATCH] D124446: [clang-tidy] Add the misc-discarded-return-value check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:181
+
+  static const auto Decltype = decltypeType(hasUnderlyingExpr(Call));
+  static const auto TemplateArg =

whisperity wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > whisperity wrote:
> > > > > whisperity wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > So, I'm not super keen on this approach of having to try to 
> > > > > > > identify every single place in which an expression is considered 
> > > > > > > to be "used" -- this is going to be fragile because we'll miss 
> > > > > > > places and it's going to be a maintenance burden because new 
> > > > > > > places will be added as the languages evolve.
> > > > > > > 
> > > > > > > For example, if we're handling `decltype` as a use, why not 
> > > > > > > `noexcept`? Or conditional `explicit`? What about a `co_return` 
> > > > > > > statement?
> > > > > > > 
> > > > > > > I'm not certain what we can do to improve this, but I think it's 
> > > > > > > worth trying to explore options to see if we can generalize what 
> > > > > > > constitutes a use so that we can write a few custom matchers to 
> > > > > > > do the heavy lifting instead of trying to play whack-a-mole.
> > > > > > I've been having other thoughts about this `decltype` here... 
> > > > > > Actually, neither `decltype` nor `noexcept` should be handled as a 
> > > > > > //"use"// at all, while `co_return` should be the same as a 
> > > > > > `return` -- however, I think it was due to lack of projects where 
> > > > > > such could be meaningfully measured as a missed case was why 
> > > > > > implementing that failed.
> > > > > > 
> > > > > > For `decltype`, `typedef`, and `noexcept` (and perhaps several 
> > > > > > others), the good solution would be having a third route: calls 
> > > > > > that //should not be counted//. Neither as a "consumed call", nor 
> > > > > > as a "bare call". Ignored, from both calculations. Maybe even for 
> > > > > > template arguments below.
> > > > > As for better matching... Unfortunately, types in the AST are so 
> > > > > varied and `hasDescendant` is too generic to express something like 
> > > > > `stmt(anyOf(ifStmt(), forStmt(), switchStmt()), hasDescendant(Call))` 
> > > > > to express in a single expression matching uses... The conditions are 
> > > > > not always direct children of the outer node, while `hasDescendant` 
> > > > > will match not just the condition but the entire tree... resulting in 
> > > > > things like //both// functions in
> > > > > 
> > > > > ```lang=cpp
> > > > > if (foo())
> > > > >   bar()
> > > > > ```
> > > > > 
> > > > > matching.
> > > > > 
> > > > > Well... generalisation... I can throw in a formal fluke:
> > > > > 
> > > > > > A **use** is a //context// for a specific `CallExpr C` in which we 
> > > > > > can reasonably assume that the value produced by evaluating `C` is 
> > > > > > loaded by another expression.
> > > > > 
> > > > > Now what I found is `-Wunused-result`, aka 
> > > > > `SemaDiagnostics::warn_unused_expr`, which is triggered in the 
> > > > > function `ExprResult Sema::ActOnFinishFullExpr(Expr* FE, 
> > > > > SourceLocation CC, bool DiscardedValue, bool IsConstexpr);`. Now this 
> > > > > function itself does //some// heuristics inside (with a **lot** of 
> > > > > `FIXME`s as of rGdab5e10ea5dbc2e6314e0e7ce54a9c51fbcb44bd), but 
> > > > > notably, `DiscardedValue` is a parameter. According to a quick 
> > > > > search, this function (and its overloads) have **82** callsites 
> > > > > within `Sema`, with many of them just tougher to decipher than 
> > > > > others. Some of the other ways this function is called, e.g. 
> > > > > `ActOnStmtExprResult`, have codes like this:
> > > > > 
> > > > > ```lang=cpp
> > > > > IsStmtExprResult = GetLookAheadToken(LookAhead).is(tok::r_brace) && 
> > > > > GetLookAheadToken(LookAhead + 1).is(tok::r_paren);
> > > > > ```
> > > > > 
> > > > > So I would say most of the logic there is **very** parsing specific, 
> > > > > and requires information that is only available during the parsing 
> > > > > descent, and not later when someone tries to consume a `const AST`.
> > > > @aaron.ballman There is a `bugprone-unused-return-value` since mid 
> > > > 2018, in which the matched function set is configurable with a 
> > > > hardcoded default, and the matching logic is also... verbose.
> > > > 
> > > > [[ 
> > > > http://github.com/llvm/llvm-project/blob/c1a9d14982f887355da1959eba3a47b952fc6e7a/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp#L144-L165
> > > >  | Source ]]
> > > > 
> > > > ```lang=cpp
> > > > auto UnusedInIfStmt =
> > > >   ifStmt(eachOf(hasThen(MatchedCallExpr), 
> > > > hasElse(MatchedCallExpr)));
> > > >   auto UnusedInWhileStmt = whileStmt(hasBody(MatchedCallExpr));
> > > >   auto UnusedInDoStmt = doStmt(hasBody(MatchedCallExpr));
> > > > ```
> > > > 
> 

[PATCH] D124446: [clang-tidy] Add the misc-discarded-return-value check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:181
+
+  static const auto Decltype = decltypeType(hasUnderlyingExpr(Call));
+  static const auto TemplateArg =

aaron.ballman wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > whisperity wrote:
> > > > whisperity wrote:
> > > > > aaron.ballman wrote:
> > > > > > So, I'm not super keen on this approach of having to try to 
> > > > > > identify every single place in which an expression is considered to 
> > > > > > be "used" -- this is going to be fragile because we'll miss places 
> > > > > > and it's going to be a maintenance burden because new places will 
> > > > > > be added as the languages evolve.
> > > > > > 
> > > > > > For example, if we're handling `decltype` as a use, why not 
> > > > > > `noexcept`? Or conditional `explicit`? What about a `co_return` 
> > > > > > statement?
> > > > > > 
> > > > > > I'm not certain what we can do to improve this, but I think it's 
> > > > > > worth trying to explore options to see if we can generalize what 
> > > > > > constitutes a use so that we can write a few custom matchers to do 
> > > > > > the heavy lifting instead of trying to play whack-a-mole.
> > > > > I've been having other thoughts about this `decltype` here... 
> > > > > Actually, neither `decltype` nor `noexcept` should be handled as a 
> > > > > //"use"// at all, while `co_return` should be the same as a `return` 
> > > > > -- however, I think it was due to lack of projects where such could 
> > > > > be meaningfully measured as a missed case was why implementing that 
> > > > > failed.
> > > > > 
> > > > > For `decltype`, `typedef`, and `noexcept` (and perhaps several 
> > > > > others), the good solution would be having a third route: calls that 
> > > > > //should not be counted//. Neither as a "consumed call", nor as a 
> > > > > "bare call". Ignored, from both calculations. Maybe even for template 
> > > > > arguments below.
> > > > As for better matching... Unfortunately, types in the AST are so varied 
> > > > and `hasDescendant` is too generic to express something like 
> > > > `stmt(anyOf(ifStmt(), forStmt(), switchStmt()), hasDescendant(Call))` 
> > > > to express in a single expression matching uses... The conditions are 
> > > > not always direct children of the outer node, while `hasDescendant` 
> > > > will match not just the condition but the entire tree... resulting in 
> > > > things like //both// functions in
> > > > 
> > > > ```lang=cpp
> > > > if (foo())
> > > >   bar()
> > > > ```
> > > > 
> > > > matching.
> > > > 
> > > > Well... generalisation... I can throw in a formal fluke:
> > > > 
> > > > > A **use** is a //context// for a specific `CallExpr C` in which we 
> > > > > can reasonably assume that the value produced by evaluating `C` is 
> > > > > loaded by another expression.
> > > > 
> > > > Now what I found is `-Wunused-result`, aka 
> > > > `SemaDiagnostics::warn_unused_expr`, which is triggered in the function 
> > > > `ExprResult Sema::ActOnFinishFullExpr(Expr* FE, SourceLocation CC, bool 
> > > > DiscardedValue, bool IsConstexpr);`. Now this function itself does 
> > > > //some// heuristics inside (with a **lot** of `FIXME`s as of 
> > > > rGdab5e10ea5dbc2e6314e0e7ce54a9c51fbcb44bd), but notably, 
> > > > `DiscardedValue` is a parameter. According to a quick search, this 
> > > > function (and its overloads) have **82** callsites within `Sema`, with 
> > > > many of them just tougher to decipher than others. Some of the other 
> > > > ways this function is called, e.g. `ActOnStmtExprResult`, have codes 
> > > > like this:
> > > > 
> > > > ```lang=cpp
> > > > IsStmtExprResult = GetLookAheadToken(LookAhead).is(tok::r_brace) && 
> > > > GetLookAheadToken(LookAhead + 1).is(tok::r_paren);
> > > > ```
> > > > 
> > > > So I would say most of the logic there is **very** parsing specific, 
> > > > and requires information that is only available during the parsing 
> > > > descent, and not later when someone tries to consume a `const AST`.
> > > @aaron.ballman There is a `bugprone-unused-return-value` since mid 2018, 
> > > in which the matched function set is configurable with a hardcoded 
> > > default, and the matching logic is also... verbose.
> > > 
> > > [[ 
> > > http://github.com/llvm/llvm-project/blob/c1a9d14982f887355da1959eba3a47b952fc6e7a/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp#L144-L165
> > >  | Source ]]
> > > 
> > > ```lang=cpp
> > > auto UnusedInIfStmt =
> > >   ifStmt(eachOf(hasThen(MatchedCallExpr), hasElse(MatchedCallExpr)));
> > >   auto UnusedInWhileStmt = whileStmt(hasBody(MatchedCallExpr));
> > >   auto UnusedInDoStmt = doStmt(hasBody(MatchedCallExpr));
> > > ```
> > > 
> > > Agreed, this is seemingly a subset of the inverse match.
> > > For decltype, typedef, and noexcept (and perhaps several others), the 
> > > good solution would be having a third route: calls that 

[PATCH] D125769: [clang-tidy] Introduce the WarnIntoHeaders option to modernize-deprecated-headers

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h:63
   std::vector IncludesToBeProcessed;
+  bool WarnIntoHeaders;
 };

LegalizeAdulthood wrote:
> 1) How is this different from the clang-tidy option that specifies whether or 
> not fixits are applied to header files?
> 
>   As an owner of a code base, I would know which header files are included 
> from C source files and I would set my header-file regex (honestly, not a fan 
> of a regex for that option; I'd prefer white/black lists, but that's another 
> discussion) to exclude header files that are known to be included in C source 
> files.
> 
> 2) Assuming that the header-file regex is somehow insufficient to cover this 
> scenario, I like the functionality but the name of this option feels "off".  
> (Naming things is hard.)  Elsewhere we have options that say `HeaderFile` not 
> `Headers` and `Into` just doesn't sound like the way normal conversation 
> would state the situation.  Something like `CheckHeaderFile` would be more 
> consistent with existing options.
I do not know the answer to question //1//, but as an owner of a code-base, I 
would not want to know or deal with keeping additional lists (be it 
file-by-file, glob expressions, or regex...) specific to what headers are 
includable from C and what aren't. Especially considering that every C header 
could be included from C++. (Note how LLVM uses `.h` instead of `.hpp` for its 
headers, even though >90% of our headers would never compile in C mode...) If a 
check is misbehaving for my codebase, I'd just simply disable that check (1 
line of code) and go on with my life, instead of creating a curated list, that 
is //at least// 2 LoC in a config file, //and// has to be maintained down the 
line...

Moreover, the `-header-filter` regex and `-system-headers` are Tidy-level 
global flags. This means that I would either have to let go of EVERY potential 
diagnostic that might be placed in headers, or tinker with my environment to 
run multiple instances of Clang-Tidy with different configurations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125769

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

[PATCH] D125771: [clang-tidy] Add a useful note about -std=c++11-or-later

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D125771#3519606 , 
@LegalizeAdulthood wrote:

> I thought there wasn't any support for validating fixits applied to header 
> files?

It is not specifically about the fixits, but diagnostics as a whole. It was not 
clear that if you say `-std=c++11-or-later` it will actually run **multiple** 
executions without giving you the ability to observe the contents of the files.

Technically, you can always match a diagnostic that is positioned into a header 
from the TU that is producing that diagnostic, because `// CHECK-MESSAGES:` 
does a regular expression-like matching. For example, `misc-unused-parameters` 
does the following 
:

  #include "header.h"
  // CHECK-MESSAGES: header.h:1:38: warning

And because LIT can execute arbitrary commands, you can always go about your 
way running `FileCheck` and everything else manually.

But it is good to be reminded early on that `-or-later` will result in 
execution of potentially changed code outside of your (test file's) control. 
Saves the debugging time which we had to endure!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125771

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

[PATCH] D124446: [clang-tidy] Add the misc-discarded-return-value check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:181
+
+  static const auto Decltype = decltypeType(hasUnderlyingExpr(Call));
+  static const auto TemplateArg =

whisperity wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > So, I'm not super keen on this approach of having to try to identify 
> > > every single place in which an expression is considered to be "used" -- 
> > > this is going to be fragile because we'll miss places and it's going to 
> > > be a maintenance burden because new places will be added as the languages 
> > > evolve.
> > > 
> > > For example, if we're handling `decltype` as a use, why not `noexcept`? 
> > > Or conditional `explicit`? What about a `co_return` statement?
> > > 
> > > I'm not certain what we can do to improve this, but I think it's worth 
> > > trying to explore options to see if we can generalize what constitutes a 
> > > use so that we can write a few custom matchers to do the heavy lifting 
> > > instead of trying to play whack-a-mole.
> > I've been having other thoughts about this `decltype` here... Actually, 
> > neither `decltype` nor `noexcept` should be handled as a //"use"// at all, 
> > while `co_return` should be the same as a `return` -- however, I think it 
> > was due to lack of projects where such could be meaningfully measured as a 
> > missed case was why implementing that failed.
> > 
> > For `decltype`, `typedef`, and `noexcept` (and perhaps several others), the 
> > good solution would be having a third route: calls that //should not be 
> > counted//. Neither as a "consumed call", nor as a "bare call". Ignored, 
> > from both calculations. Maybe even for template arguments below.
> As for better matching... Unfortunately, types in the AST are so varied and 
> `hasDescendant` is too generic to express something like 
> `stmt(anyOf(ifStmt(), forStmt(), switchStmt()), hasDescendant(Call))` to 
> express in a single expression matching uses... The conditions are not always 
> direct children of the outer node, while `hasDescendant` will match not just 
> the condition but the entire tree... resulting in things like //both// 
> functions in
> 
> ```lang=cpp
> if (foo())
>   bar()
> ```
> 
> matching.
> 
> Well... generalisation... I can throw in a formal fluke:
> 
> > A **use** is a //context// for a specific `CallExpr C` in which we can 
> > reasonably assume that the value produced by evaluating `C` is loaded by 
> > another expression.
> 
> Now what I found is `-Wunused-result`, aka 
> `SemaDiagnostics::warn_unused_expr`, which is triggered in the function 
> `ExprResult Sema::ActOnFinishFullExpr(Expr* FE, SourceLocation CC, bool 
> DiscardedValue, bool IsConstexpr);`. Now this function itself does //some// 
> heuristics inside (with a **lot** of `FIXME`s as of 
> rGdab5e10ea5dbc2e6314e0e7ce54a9c51fbcb44bd), but notably, `DiscardedValue` is 
> a parameter. According to a quick search, this function (and its overloads) 
> have **82** callsites within `Sema`, with many of them just tougher to 
> decipher than others. Some of the other ways this function is called, e.g. 
> `ActOnStmtExprResult`, have codes like this:
> 
> ```lang=cpp
> IsStmtExprResult = GetLookAheadToken(LookAhead).is(tok::r_brace) && 
> GetLookAheadToken(LookAhead + 1).is(tok::r_paren);
> ```
> 
> So I would say most of the logic there is **very** parsing specific, and 
> requires information that is only available during the parsing descent, and 
> not later when someone tries to consume a `const AST`.
@aaron.ballman There is a `bugprone-unused-return-value` since mid 2018, in 
which the matched function set is configurable with a hardcoded default, and 
the matching logic is also... verbose.

[[ 
http://github.com/llvm/llvm-project/blob/c1a9d14982f887355da1959eba3a47b952fc6e7a/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp#L144-L165
 | Source ]]

```lang=cpp
auto UnusedInIfStmt =
  ifStmt(eachOf(hasThen(MatchedCallExpr), hasElse(MatchedCallExpr)));
  auto UnusedInWhileStmt = whileStmt(hasBody(MatchedCallExpr));
  auto UnusedInDoStmt = doStmt(hasBody(MatchedCallExpr));
```

Agreed, this is seemingly a subset of the inverse match.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124446

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

[PATCH] D124446: [clang-tidy] Add the misc-discarded-return-value check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:83-86
+void DiscardedReturnValueCheck::onStartOfTranslationUnit() {
+  ConsumedCalls.clear();
+  CallMap.clear();
+}

aaron.ballman wrote:
> Is this code necessary?
Yes, if you have checks that store TU-specific data, and you run `clang-tidy 
a.cpp b.cpp` then if you do not clear the data structures, dangling pointers 
into already destroyed ASTs will remain.



Comment at: clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:181
+
+  static const auto Decltype = decltypeType(hasUnderlyingExpr(Call));
+  static const auto TemplateArg =

aaron.ballman wrote:
> So, I'm not super keen on this approach of having to try to identify every 
> single place in which an expression is considered to be "used" -- this is 
> going to be fragile because we'll miss places and it's going to be a 
> maintenance burden because new places will be added as the languages evolve.
> 
> For example, if we're handling `decltype` as a use, why not `noexcept`? Or 
> conditional `explicit`? What about a `co_return` statement?
> 
> I'm not certain what we can do to improve this, but I think it's worth trying 
> to explore options to see if we can generalize what constitutes a use so that 
> we can write a few custom matchers to do the heavy lifting instead of trying 
> to play whack-a-mole.
I've been having other thoughts about this `decltype` here... Actually, neither 
`decltype` nor `noexcept` should be handled as a //"use"// at all, while 
`co_return` should be the same as a `return` -- however, I think it was due to 
lack of projects where such could be meaningfully measured as a missed case was 
why implementing that failed.

For `decltype`, `typedef`, and `noexcept` (and perhaps several others), the 
good solution would be having a third route: calls that //should not be 
counted//. Neither as a "consumed call", nor as a "bare call". Ignored, from 
both calculations. Maybe even for template arguments below.



Comment at: clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:181
+
+  static const auto Decltype = decltypeType(hasUnderlyingExpr(Call));
+  static const auto TemplateArg =

whisperity wrote:
> aaron.ballman wrote:
> > So, I'm not super keen on this approach of having to try to identify every 
> > single place in which an expression is considered to be "used" -- this is 
> > going to be fragile because we'll miss places and it's going to be a 
> > maintenance burden because new places will be added as the languages evolve.
> > 
> > For example, if we're handling `decltype` as a use, why not `noexcept`? Or 
> > conditional `explicit`? What about a `co_return` statement?
> > 
> > I'm not certain what we can do to improve this, but I think it's worth 
> > trying to explore options to see if we can generalize what constitutes a 
> > use so that we can write a few custom matchers to do the heavy lifting 
> > instead of trying to play whack-a-mole.
> I've been having other thoughts about this `decltype` here... Actually, 
> neither `decltype` nor `noexcept` should be handled as a //"use"// at all, 
> while `co_return` should be the same as a `return` -- however, I think it was 
> due to lack of projects where such could be meaningfully measured as a missed 
> case was why implementing that failed.
> 
> For `decltype`, `typedef`, and `noexcept` (and perhaps several others), the 
> good solution would be having a third route: calls that //should not be 
> counted//. Neither as a "consumed call", nor as a "bare call". Ignored, from 
> both calculations. Maybe even for template arguments below.
As for better matching... Unfortunately, types in the AST are so varied and 
`hasDescendant` is too generic to express something like `stmt(anyOf(ifStmt(), 
forStmt(), switchStmt()), hasDescendant(Call))` to express in a single 
expression matching uses... The conditions are not always direct children of 
the outer node, while `hasDescendant` will match not just the condition but the 
entire tree... resulting in things like //both// functions in

```lang=cpp
if (foo())
  bar()
```

matching.

Well... generalisation... I can throw in a formal fluke:

> A **use** is a //context// for a specific `CallExpr C` in which we can 
> reasonably assume that the value produced by evaluating `C` is loaded by 
> another expression.

Now what I found is `-Wunused-result`, aka `SemaDiagnostics::warn_unused_expr`, 
which is triggered in the function `ExprResult Sema::ActOnFinishFullExpr(Expr* 
FE, SourceLocation CC, bool DiscardedValue, bool IsConstexpr);`. Now this 
function itself does //some// heuristics inside (with a **lot** of `FIXME`s as 
of rGdab5e10ea5dbc2e6314e0e7ce54a9c51fbcb44bd), but notably, `DiscardedValue` 
is a parameter. According to a quick search, this function (and its overloads) 
have **82** callsites 

[PATCH] D125383: [ASTMatchers][clang-tidy][NFC] Hoist 'forEachTemplateArgument' matcher into the core library

[Whisperity via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9add949557d2: [ASTMatchers][clang-tidy][NFC] Hoist 
`forEachTemplateArgument` matcher into the… (authored by whisperity).

Changed prior to commit:
  https://reviews.llvm.org/D125383?vs=429161=429192#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125383

Files:
  clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
  clang/docs/LibASTMatchersReference.html
  clang/docs/ReleaseNotes.rst
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -4971,6 +4971,62 @@
 std::make_unique>("if", 6)));
 }
 
+TEST(ForEachTemplateArgument, OnFunctionDecl) {
+  const std::string Code = R"(
+template  void f(T, U) {}
+void test() {
+  int I = 1;
+  bool B = false;
+  f(I, B);
+})";
+  EXPECT_TRUE(matches(
+  Code, functionDecl(forEachTemplateArgument(refersToType(builtinType(,
+  langCxx11OrLater()));
+  auto matcher =
+  functionDecl(forEachTemplateArgument(
+   templateArgument(refersToType(builtinType().bind("BT")))
+   .bind("TA")))
+  .bind("FN");
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("FN", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("TA", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("BT", 2)));
+}
+
+TEST(ForEachTemplateArgument, OnClassTemplateSpecialization) {
+  const std::string Code = R"(
+template 
+struct Matrix {};
+
+static constexpr unsigned R = 2;
+
+Matrix M;
+)";
+  EXPECT_TRUE(matches(
+  Code, templateSpecializationType(forEachTemplateArgument(isExpr(expr(,
+  langCxx11OrLater()));
+  auto matcher = templateSpecializationType(
+ forEachTemplateArgument(
+ templateArgument(isExpr(expr().bind("E"))).bind("TA")))
+ .bind("TST");
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("TST",
+  2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("TA", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher, std::make_unique>("E", 2)));
+}
+
 TEST(Has, DoesNotDeleteBindings) {
   EXPECT_TRUE(matchAndVerifyResultTrue(
 "class X { int a; };", recordDecl(decl().bind("x"), has(fieldDecl())),
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -250,6 +250,7 @@
   REGISTER_MATCHER(forEachLambdaCapture);
   REGISTER_MATCHER(forEachOverridden);
   REGISTER_MATCHER(forEachSwitchCase);
+  REGISTER_MATCHER(forEachTemplateArgument);
   REGISTER_MATCHER(forField);
   REGISTER_MATCHER(forFunction);
   REGISTER_MATCHER(forStmt);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -5011,6 +5011,49 @@
   return Node.getNumParams() == N;
 }
 
+/// Matches classTemplateSpecialization, templateSpecializationType and
+/// functionDecl nodes where the template argument matches the inner matcher.
+/// This matcher may produce multiple matches.
+///
+/// Given
+/// \code
+///   template 
+///   struct Matrix {};
+///
+///   constexpr unsigned R = 2;
+///   Matrix M;
+///
+///   template 
+///   void f(T&& t, U&& u) {}
+///
+///   bool B = false;
+///   f(R, B);
+/// \endcode
+/// templateSpecializationType(forEachTemplateArgument(isExpr(expr(
+///   matches twice, with expr() matching 'R * 2' and 'R * 4'
+/// functionDecl(forEachTemplateArgument(refersToType(builtinType(
+///   matches the specialization f twice, for 'unsigned'
+///   and 'bool'
+AST_POLYMORPHIC_MATCHER_P(
+forEachTemplateArgument,
+AST_POLYMORPHIC_SUPPORTED_TYPES(ClassTemplateSpecializationDecl,
+TemplateSpecializationType, FunctionDecl),
+clang::ast_matchers::internal::Matcher, InnerMatcher) {
+  ArrayRef TemplateArgs =
+  clang::ast_matchers::internal::getTemplateSpecializationArgs(Node);
+  clang::ast_matchers::internal::BoundNodesTreeBuilder Result;
+  bool Matched = false;
+  for (const auto  : TemplateArgs) {
+

[PATCH] D125383: [ASTMatchers][clang-tidy][NFC] Hoist 'forEachTemplateArgument' matcher into the core library

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D125383#3509084 , @aaron.ballman 
wrote:

> Then I think the only thing missing here are updates to 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/ASTMatchers/Dynamic/Registry.cpp.

Could you please check if I did it the right way? I did not know about this 
file, and I am still not exactly sure about its purpose.


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

https://reviews.llvm.org/D125383

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

[PATCH] D125383: [ASTMatchers][clang-tidy][NFC] Hoist 'forEachTemplateArgument' matcher into the core library

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 429161.
whisperity added a comment.

- Added to `ASTMatchers/Registry.cpp`
- Updated with release notes


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

https://reviews.llvm.org/D125383

Files:
  clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
  clang/docs/LibASTMatchersReference.html
  clang/docs/ReleaseNotes.rst
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -4971,6 +4971,63 @@
 std::make_unique>("if", 6)));
 }
 
+TEST(ForEachTemplateArgument, OnFunctionDecl) {
+  const std::string Code = R"(
+template  void f(T, U) {}
+void test() {
+  int I = 1;
+  bool B = false;
+  f(I, B);
+})";
+  EXPECT_TRUE(matches(
+  Code, functionDecl(forEachTemplateArgument(refersToType(builtinType(,
+  langCxx11OrLater()));
+  auto matcher =
+  functionDecl(forEachTemplateArgument(
+   templateArgument(refersToType(builtinType().bind("BT")))
+   .bind("TA")))
+  .bind("FN");
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("FN", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("TA", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("BT", 2)));
+}
+
+TEST(ForEachTemplateArgument, OnClassTemplateSpecialization) {
+  const std::string Code = R"(
+template 
+struct Matrix {};
+
+static constexpr unsigned R = 2;
+
+Matrix M;
+)";
+  EXPECT_TRUE(matches(
+  Code, templateSpecializationType(forEachTemplateArgument(isExpr(expr(,
+  langCxx11OrLater()));
+  auto matcher = templateSpecializationType(
+ forEachTemplateArgument(
+ templateArgument(isExpr(expr().bind("E"))).bind("TA")))
+ .bind("TST");
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("TST",
+  2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("TA", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("E", 2)));
+}
+
 TEST(Has, DoesNotDeleteBindings) {
   EXPECT_TRUE(matchAndVerifyResultTrue(
 "class X { int a; };", recordDecl(decl().bind("x"), has(fieldDecl())),
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -250,6 +250,7 @@
   REGISTER_MATCHER(forEachLambdaCapture);
   REGISTER_MATCHER(forEachOverridden);
   REGISTER_MATCHER(forEachSwitchCase);
+  REGISTER_MATCHER(forEachTemplateArgument);
   REGISTER_MATCHER(forField);
   REGISTER_MATCHER(forFunction);
   REGISTER_MATCHER(forStmt);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -5011,6 +5011,49 @@
   return Node.getNumParams() == N;
 }
 
+/// Matches classTemplateSpecialization, templateSpecializationType and
+/// functionDecl nodes where the template argument matches the inner matcher.
+/// This matcher may produce multiple matches.
+///
+/// Given
+/// \code
+///   template 
+///   struct Matrix {};
+///
+///   constexpr unsigned R = 2;
+///   Matrix M;
+///
+///   template 
+///   void f(T&& t, U&& u) {}
+///
+///   bool B = false;
+///   f(R, B);
+/// \endcode
+/// templateSpecializationType(forEachTemplateArgument(isExpr(expr(
+///   matches twice, with expr() matching 'R * 2' and 'R * 4'
+/// functionDecl(forEachTemplateArgument(refersToType(builtinType(
+///   matches the specialization f twice, for 'unsigned'
+///   and 'bool'
+AST_POLYMORPHIC_MATCHER_P(
+forEachTemplateArgument,
+AST_POLYMORPHIC_SUPPORTED_TYPES(ClassTemplateSpecializationDecl,
+TemplateSpecializationType, FunctionDecl),
+clang::ast_matchers::internal::Matcher, InnerMatcher) {
+  ArrayRef TemplateArgs =
+  clang::ast_matchers::internal::getTemplateSpecializationArgs(Node);
+  clang::ast_matchers::internal::BoundNodesTreeBuilder Result;
+  bool Matched = false;
+  for (const auto  : TemplateArgs) {
+clang::ast_matchers::internal::BoundNodesTreeBuilder ArgBuilder(*Builder);
+if (InnerMatcher.matches(Arg, Finder, )) {
+  Matched = true;
+  Result.addMatch(ArgBuilder);
+}
+  }
+  *Builder = std::move(Result);
+  return Matched;
+}
+
 

[PATCH] D125383: [ASTMatchers][clang-tidy][NFC] Hoist 'forEachTemplateArgument' matcher into the core library

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D125383#3508960 , @aaron.ballman 
wrote:

> Do you expect to use this matcher in a new check in the immediate future?

D124446  would like to use it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125383

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

[PATCH] D125209: [clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

(Also: this is a fix to an issue of your own finding and not something that was 
reported on Bugzilla? Just to make sure we cross-reference and close tickets.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125209

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

[PATCH] D125209: [clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

LGTM. Some typos inline. Also I think this is a significant enough bug-fix that 
it warrants an entry in the Release Notes under `Changes to existing checks`.

Also, the tests are failing, but it fails on the formatting of the test file 
(???), and not the actual test itself. (Usually such issues would be sent back 
to Phab to appear an a machine-made inline comment, I am not sure what happened 
to that logic.)

  changed files:
  
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp




Comment at: 
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:79-82
+auto Begin = IncludesToBeProcessed.begin();
+auto End = IncludesToBeProcessed.end();
+auto Low = std::lower_bound(Begin, End, ExternCBlockBegin);
+auto Up = std::upper_bound(Low, End, ExternCBlockEnd);

(`llvm/ADT/STLExtras.h` define a range-based `lower_bound` and `upper_bound`)



Comment at: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:98
+  // to act on a "TranslationUnit" to acquire the AST where we can walk each
+  // decl and look for `extern "C"` blocks where we wil lsuppress the report we
+  // collected during the preprocessing phase.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125209

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

[PATCH] D123773: [clang][analyzer][ctu] Make CTU a two phase analysis

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang/include/clang/AST/ASTImporterSharedState.h:83
+
+  void setNewDecl(Decl *ToD) { NewDecls.insert(ToD); }
 };

(The naming of this function feels a bit odd. `markAsNewDecl` or just 
`markNewDecl`?)



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:413
+unsigned, CTUMaxNodesMultiplier, "ctu-max-nodes-mul",
+"We count the nodes for a normal single tu analysis. We multiply that "
+"number with this config value then we divide the result by 100 to get "





Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:413-419
+"We count the nodes for a normal single tu analysis. We multiply that "
+"number with this config value then we divide the result by 100 to get "
+"the maximum number of nodes that the analyzer can generate while "
+"exploring a top level function in CTU mode (for each exploded graph)."
+"For example, 100 means that CTU will explore maximum as much nodes that "
+"we explored during the single tu mode, 200 means it will explore twice as 
"
+"much, 50 means it will explore maximum 50% more.", 100)

whisperity wrote:
> 
Couldn't this description here be simplified to say something along the lines 
of //"the percentage of single-TU analysed nodes that the CTU analysis is 
allowed to visit"//? Is the calculation method needed from the user's 
perspective? The examples talk about percentage too.



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:141
 
+enum class CTUPhase1InliningKind { None, Small, All };
+

Is this configuration inherent to the static analyser, and not the //CrossTU// 
library?



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:141
 
+enum class CTUPhase1InliningKind { None, Small, All };
+

whisperity wrote:
> Is this configuration inherent to the static analyser, and not the 
> //CrossTU// library?
(Documentation for the options are missing.)



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:156
   llvm::PointerUnion Origin;
+  mutable Optional Foreign; // From CTU.
 





Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:814-816
+  /// Returns true if the CTU analysis is running its first phase.
+  /// Returns true in single TU (non-CTU) mode!
+  bool isCTUInFirtstPhase() { return Engine.getCTUWorkList(); }

How and why is this needed? Could you call it `isSingleTUOr1stPhaseCTU` instead?

Rather, if this is used as a distinction input in conditionals, could you 
invert the branches and have a function `isSecondPhaseCTU`, and do the inverted 
logic where this function is consumed?



Comment at: 
clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp.externalDefMap.ast-dump.txt:1
+11:c:@F@other# ctu-onego-other.cpp.ast

Why is there only 1 symbol in this file, when the file above contains two 
function definitions?



Comment at: clang/test/Analysis/ctu-on-demand-parsing.c:22
 //
+// FIXME On-demand ctu should be tested in the same file that we have for the
+// PCH version, but with a different verify prefix (e.g. -verfiy=on-demand-ctu)





Comment at: clang/test/Analysis/ctu-on-demand-parsing.cpp:33
+
+// FIXME On-demand ctu should be tested in the same file that we have for the
+// PCH version, but with a different verify prefix (e.g. -verfiy=on-demand-ctu)





Comment at: clang/test/Analysis/ctu-onego-toplevel.cpp:24
+// RUN:   -verify=ctu %s 2>&1 | FileCheck %s
+//
+// CallGraph: c->b

Are the lines below related to the execution of the command above? If not, 
could you please break the comment?



Comment at: clang/test/Analysis/ctu-onego-toplevel.cpp:28
+// Note that `other` calls into `b` but that is not visible in the CallGraph
+// b/c that happens in another TU.
+





Comment at: clang/test/Analysis/ctu-onego-toplevel.cpp:30
+
+// During the onego CTU analysis, we start with c() as top level function.
+// Then we visit b() as non-toplevel during the processing of the FWList, thus

One-go? And what does that refer to? Is "Onego" analysis the one this patch is 
introducing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123773

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

[PATCH] D125209: [clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:135
 {"float.h", "cfloat"},
 {"limits.h", "climits"},
 {"locale.h", "clocale"},

steakhal wrote:
> whisperity wrote:
> > POSIX defines a "global" include `limits.h` which gives you the definition 
> > of macros like `PATH_MAX`. Will such code keep on working if the include is 
> > turned into `climits`?
> IDK.
Did an investigation on Ubuntu with G++ 7.5. So it turns out that the GNU G++ 
standard library implementation, on Linux, is implemented in a way that it will 
include the POSIX-specific `limits.h` into the C Standard `limits.h` and thus 
also `climits`.

And
> Out of these projects 
> llvm-project,**contour**,codechecker,**qtbase**,protobuf,bitcoin,xerces,libwebm,tinyxml2,postgres,ffmpeg,sqlite,openssl,vim,twin,curl,**tmux**,memcached,
>  it suppressed 5 reports

Well, Contour, tmux and Qt (and perhaps even LLVM's OS-specific support lib) 
//SHOULD// be heavily reliant on interfacing with POSIX stuff when compiled 
against Linux, so I guess we can say this transformation is not dangerous.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125209

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

[PATCH] D125209: [clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:87-88
 const SourceManager , Preprocessor *PP, Preprocessor *ModuleExpanderPP) 
{
-PP->addPPCallbacks(
-::std::make_unique(*this, getLangOpts()));
+  PP->addPPCallbacks(::std::make_unique(
+  *this, getLangOpts(), PP->getSourceManager()));
+}

steakhal wrote:
> whisperity wrote:
> > (: Same here. Is this state that might keep a dangling reference if 
> > multiple TUs are consumed in sequence?)
> How can I acquire the right `SourceManager` if not like this?
> I haven't found any alternative.
> What do you suggest?
Is the `PP` a different variable when a new translation unit is consumed by the 
same check instance? If so, then there is no problem. A simple debug print 
`llvm::errs() << PP << '\n';`, recompile, re-run with `clang-tidy a.cpp b.cpp` 
should show how the infrastructure behaves. I do not know the exact behaviour 
of the preprocessor layer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125209

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

[PATCH] D125209: [clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:70-73
+auto ExternCBlockBegin =
+SM.getDecomposedExpansionLoc(LinkSpecDecl->getBeginLoc());
+auto ExternCBlockEnd =
+SM.getDecomposedExpansionLoc(LinkSpecDecl->getEndLoc());

This unconditionally assumes that an encountered LSD will be `extern "C"`. 
However, `extern "C++"` is also possible (and supported by Clang). (And things 
like `extern "Java"` are also possible according to the standard, albeit not 
supported by Clang.)



Comment at: 
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:87-88
 const SourceManager , Preprocessor *PP, Preprocessor *ModuleExpanderPP) 
{
-PP->addPPCallbacks(
-::std::make_unique(*this, getLangOpts()));
+  PP->addPPCallbacks(::std::make_unique(
+  *this, getLangOpts(), PP->getSourceManager()));
+}

(: Same here. Is this state that might keep a dangling reference if multiple 
TUs are consumed in sequence?)



Comment at: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:92
+ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(ast_matchers::translationUnitDecl().bind("TU"), this);
+}

(Perhaps it is worth an explicit comment here that you do this at first glance 
unconventional match because the entire check's logic is calculated **only** on 
the "preprocessor" level.)



Comment at: 
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:135
 {"float.h", "cfloat"},
 {"limits.h", "climits"},
 {"locale.h", "clocale"},

POSIX defines a "global" include `limits.h` which gives you the definition of 
macros like `PATH_MAX`. Will such code keep on working if the include is turned 
into `climits`?



Comment at: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h:66
+  friend class detail::ExternCRefutationVisitor;
+  std::vector IncludesToBeProcessed;
 };

: You are storing some TU-specific state here (`SourceRange`) that is not 
cleared. What happens if you run `clang-tidy a.cpp b.cpp`, i.e. two TUs in the 
same process execution?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125209

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

[PATCH] D124447: [clang-tidy] Add infrastructure support for running on project-level information

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D124447#3506536 , @whisperity 
wrote:

> In D124447#3493996 , @whisperity 
> wrote:
>
>> In D124447#3493446 , 
>> @aaron.ballman wrote:
>>
>>> precommit CI is showing a fair amount of failures that I believe are 
>>> related to your patch.
>>
>> I have no idea what is causing the CI issues, because all the CI issues are 
>> related to unit test libraries removing `const` from fixits and such(??) 
>> which this patch (or any in the current patch set) doesn't even touch, at 
>> all. I did run the test targets locally... it's likely that simply the 
>> rebase and the push happened against an unclean/breaking main branch...
>
> Rebase & rerun against a current `main` branch produces the issues locally 
> for me, too. So far I have no idea what might be causing this, but I'll keep 
> on digging. However, let's continue with discussing the general approach 
> meanwhile. 

Actually, it turned out to be easy after all! The implementation **is** 
backwards compatible //on the command-line// but was not for the 
"machine-driven" tests that are implemented as C++ code and execution scaffold. 
Adding the below commented change to the diff **completely** solved the failing 
tests for me locally (turns out none of the checks were actually running the 
`check()` callback!), but I cannot update a //"Revision"// that I do not own.




Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:60
+
+  MultipassProjectPhase MultipassPhase;
+  /// The directory where multi-pass project-level analysis stores its data to.

⚠ The crucial fix against the unit-test failures!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124447

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

[PATCH] D118996: [clang-tidy] Support C++14 in bugprone-signal-handler.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@njames93 What do you think about the current approach? It will 
under-approximate the problem-inducing node set but at least cover what we know 
about C++ now.

(@balazske please mark //"Done"// the answered discussion threads.)




Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:284-296
+  return isa(S) || isa(S) ||
+ isa(S) || isa(S) ||
+ isa(S) || isa(S) ||
+ isa(S) || isa(S) ||
+ isa(S) || isa(S) ||
+ isa(S) || isa(S) ||
+ isa(S) || isa(S) ||

`isa<>` is a variadic template since a few major releases ago, i.e. `isa(X)` is supported too.

(Also, please order this alphabetically, so it's easier to read and insert into 
at later edits.)



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:285
+  return isa(S) || isa(S) ||
+ isa(S) || isa(S) ||
+ isa(S) || isa(S) ||

Is CUDA built upon C++-specific features?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118996

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Just one question if you could try this out for me: what happens if you run 
`clang-tidy a.c b.c` (two TUs in the invocation) where **one of them** 
(preferably the later one, i.e. **`b.c`**) does //NOT// have Annex K enabled? I 
believe the cached `IsAnnexKAvailable` (like any other TU-specific state of the 
check instance) should be invalidated/cleared in an overridden `void 
onStartTranslationUnit()` function.

Also, what happens if the check is run for C++ code?




Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:208-214
+
+  ClangTidyOptions getModuleOptions() override {
+ClangTidyOptions Options;
+auto  = Options.CheckOptions;
+Opts["bugprone-unsafe-functions.ReportMoreUnsafeFunctions"] = "true";
+return Options;
+  }

What is the reason for this being a //module// option, when the name of the 
option looks like a //check// option? Did something change and the API requires 
this now? If you do `Options.get("ReportMoreUnsafeFunctions", true)` it will 
automatically work and use this default option. You also overridden the 
`storeOptions()` function properly by the looks of it, so there should be no 
reason for this change.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp:157-159
+
+  if (!getLangOpts().C11) {
+// Caching the result.

(And you can remove all the other `// Caching the result` comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp:203-212
+  Optional AnnexKReplacementFunction =
+  useSafeFunctionsFromAnnexK()
+  ? StringSwitch>(FunctionName)
+.Cases("asctime", "asctime_r", 
Optional{"asctime_s"})
+.Case("gets", Optional{"gets_s"})
+.Default(None)
+  : None;

Instead of wrapping the `StringRef` into an `Optional`, couldn't we achieve the 
same with the empty string(ref)... signalling the fact that there is no 
replacement?



Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h:21
+/// deprecated or missing bounds checking. For the listed functions a
+/// replacement function is suggested. The checker heavily relies on the
+/// functions from Annex K (Bounds-checking interfaces) of C11.





Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h:47
+
+  // Checker option.
+  const bool ReportMoreUnsafeFunctions;

(Superfluous comment.)



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:125
+  deprecated or missing bounds checking. For the listed functions a
+  replacement function is suggested. The checker heavily relies on the
+  functions from Annex K (Bounds-checking interfaces) of C11.





Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unsafe-functions.rst:9
+deprecated or missing bounds checking. For the listed functions a
+replacement function is suggested. The checker heavily relies on the
+functions from Annex K (Bounds-checking interfaces) of C11.





Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unsafe-functions.rst:81-83
+strcpy(buf, prefix); // warning: function 'strcpy' is not bounds-checking; 
'strcpy_s' should be used instead.
+strcat(buf, msg); // warning: function 'strcat' is not bounds-checking; 
'strcat_s' should be used instead.
+strcat(buf, suffix); // warning: function 'strcat' is not bounds-checking; 
'strcat_s' should be used instead.

(Visual nit.)


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

https://reviews.llvm.org/D91000

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

[PATCH] D124447: [clang-tidy] Add infrastructure support for running on project-level information

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D124447#3493996 , @whisperity 
wrote:

> In D124447#3493446 , @aaron.ballman 
> wrote:
>
>> precommit CI is showing a fair amount of failures that I believe are related 
>> to your patch.
>
> I have no idea what is causing the CI issues, because all the CI issues are 
> related to unit test libraries removing `const` from fixits and such(??) 
> which this patch (or any in the current patch set) doesn't even touch, at 
> all. I did run the test targets locally... it's likely that simply the rebase 
> and the push happened against an unclean/breaking main branch...

Rebase & rerun against a current `main` branch produces the issues locally for 
me, too. So far I have no idea what might be causing this, but I'll keep on 
digging. However, let's continue with discussing the general approach 
meanwhile. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124447

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

[PATCH] D124446: [clang-tidy] Add the misc-discarded-return-value check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:96-99
+AST_MATCHER_P(StaticAssertDecl, hasAssertExpr,
+  ast_matchers::internal::Matcher, InnerMatcher) {
+  return InnerMatcher.matches(*Node.getAssertExpr(), Finder, Builder);
+}

This is superfluous. The error message of a `static_assert` is always a string 
literal that has to be //right there// (i.e. a `constexpr const char *` 
function is NOT accepted). So if you are matching and ignoring the 
`ImplicitCastExpr`s, the following //SHOULD// match the static assert just fine:

```lang=cpp
staticAssertDecl(has(callExpr()))
```

http://godbolt.org/z/oeWfh3Mjo



Comment at: 
clang-tools-extra/clang-tidy/misc/DiscardedReturnValueCheck.cpp:106-126
+// FIXME: Move ASTMatcher library.
+// Note: Taken from UnusedUsingDeclsCheck.
+AST_POLYMORPHIC_MATCHER_P(
+forEachTemplateArgument,
+AST_POLYMORPHIC_SUPPORTED_TYPES(ClassTemplateSpecializationDecl,
+TemplateSpecializationType, FunctionDecl),
+clang::ast_matchers::internal::Matcher, InnerMatcher) {

martong wrote:
> Could you please have this in an independent parent patch?
D125383



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-discarded-return-value-50p.cpp:631-635
+  // FIXME: Because call is a template, calls to it, even with 
different
+  // lambdas are calculated together, but the resulting diagnostic message is
+  // wrong. The culprit seems to be the USR generated for
+  // 'call<(lambda in lambdaCallerTest)>' being
+  // 
"c:misc-discarded-return-value-50p.cpp@F@call<#&$@F@lambdaCallerTest#@Sa>#S0_#"

(⏰ Stale comment leaked back in time from the later cross-TU implementation. 
This patch does not involve USRs yet.)



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-discarded-return-value-50p.cpp:645-646
+
+  // Here, call<...> has a different key:
+  // 
"c:misc-discarded-return-value-50p.cpp@F@call<#&$@F@lambdaCallerTest2#@Sa>#S0_#"
+}

[⏰ Ditto]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124446

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

[PATCH] D125383: [ASTMatchers][clang-tidy][NFC] Hoist 'forEachTemplateArgument' matcher into the core library

[Whisperity via Phabricator via cfe-commits]

whisperity created this revision.
whisperity added reviewers: aaron.ballman, klimek, hokein, njames93.
whisperity added projects: clang, clang-tools-extra.
Herald added subscribers: carlosgalvezp, gamesh411, Szelethus, dkrupp, 
rnkovacs, xazax.hun.
Herald added a project: All.
whisperity requested review of this revision.

Fixes the `FIXME:` related to adding `forEachTemplateArgument` to the core 
//AST Matchers// library.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125383

Files:
  clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -4971,6 +4971,63 @@
 std::make_unique>("if", 6)));
 }
 
+TEST(ForEachTemplateArgument, OnFunctionDecl) {
+  const std::string Code = R"(
+template  void f(T, U) {}
+void test() {
+  int I = 1;
+  bool B = false;
+  f(I, B);
+})";
+  EXPECT_TRUE(matches(
+  Code, functionDecl(forEachTemplateArgument(refersToType(builtinType(,
+  langCxx11OrLater()));
+  auto matcher =
+  functionDecl(forEachTemplateArgument(
+   templateArgument(refersToType(builtinType().bind("BT")))
+   .bind("TA")))
+  .bind("FN");
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("FN", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("TA", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("BT", 2)));
+}
+
+TEST(ForEachTemplateArgument, OnClassTemplateSpecialization) {
+  const std::string Code = R"(
+template 
+struct Matrix {};
+
+static constexpr unsigned R = 2;
+
+Matrix M;
+)";
+  EXPECT_TRUE(matches(
+  Code, templateSpecializationType(forEachTemplateArgument(isExpr(expr(,
+  langCxx11OrLater()));
+  auto matcher = templateSpecializationType(
+ forEachTemplateArgument(
+ templateArgument(isExpr(expr().bind("E"))).bind("TA")))
+ .bind("TST");
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("TST",
+  2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("TA", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+  Code, matcher,
+  std::make_unique>("E", 2)));
+}
+
 TEST(Has, DoesNotDeleteBindings) {
   EXPECT_TRUE(matchAndVerifyResultTrue(
 "class X { int a; };", recordDecl(decl().bind("x"), has(fieldDecl())),
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -5011,6 +5011,49 @@
   return Node.getNumParams() == N;
 }
 
+/// Matches classTemplateSpecialization, templateSpecializationType and
+/// functionDecl nodes where the template argument matches the inner matcher.
+/// This matcher may produce multiple matches.
+///
+/// Given
+/// \code
+///   template 
+///   struct Matrix {};
+///
+///   constexpr unsigned R = 2;
+///   Matrix M;
+///
+///   template 
+///   void f(T&& t, U&& u) {}
+///
+///   bool B = false;
+///   f(R, B);
+/// \endcode
+/// templateSpecializationType(forEachTemplateArgument(isExpr(expr(
+///   matches twice, with expr() matching 'R * 2' and 'R * 4'
+/// functionDecl(forEachTemplateArgument(refersToType(builtinType(
+///   matches the specialization f twice, for 'unsigned'
+///   and 'bool'
+AST_POLYMORPHIC_MATCHER_P(
+forEachTemplateArgument,
+AST_POLYMORPHIC_SUPPORTED_TYPES(ClassTemplateSpecializationDecl,
+TemplateSpecializationType, FunctionDecl),
+clang::ast_matchers::internal::Matcher, InnerMatcher) {
+  ArrayRef TemplateArgs =
+  clang::ast_matchers::internal::getTemplateSpecializationArgs(Node);
+  clang::ast_matchers::internal::BoundNodesTreeBuilder Result;
+  bool Matched = false;
+  for (const auto  : TemplateArgs) {
+clang::ast_matchers::internal::BoundNodesTreeBuilder ArgBuilder(*Builder);
+if (InnerMatcher.matches(Arg, Finder, )) {
+  Matched = true;
+  Result.addMatch(ArgBuilder);
+}
+  }
+  *Builder = std::move(Result);
+  return Matched;
+}
+
 /// Matches \c FunctionDecls that have a noreturn attribute.
 ///
 /// Given
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -7391,6 +7391,32 @@
 
 
 

[PATCH] D124447: [clang-tidy] Add infrastructure support for running on project-level information

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D124447#3493446 , @aaron.ballman 
wrote:

> precommit CI is showing a fair amount of failures that I believe are related 
> to your patch.

It was me who helped @bahramib to rebase and split a much larger and more 
convoluted development history into broken up patches for review, and I have no 
idea what is causing the CI issues, because all the CI issues are related to 
unit test libraries removing `const` from fixits and such(??) which this patch 
(or any in the current patch set) doesn't even touch, at all. I did run the 
test targets locally... it's likely that simply the rebase and the push 
happened against an unclean/breaking main branch...

We can investigate later, but I think the review can be done either way. If 
something is related, it has to be tangential, we strived to make the entire 
new infrastructure fully backwards compatible.

The only reason I can think of is that some `LangOpts` might not be forwarded 
correctly somewhere, will definitely look into this.

(Also, full disclosure, I am the B.Sc. thesis supervisor of @bahramib so I'm 
only here to keep an eye on (and fix up the "very LLVM-y" parts) of the check, 
I cannot impartially review and accept this as I was involved in the full 
creation process. )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124447

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

[PATCH] D123065: [clang-tidy] support --load in clang-tidy-diff.py/run-clang-tidy.py

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

D'oh! I made a mistake when copying the URL and accidentally associated the 
commit with D12306  instead of D123065 
... Anyhow, this was committed in 
rGb1f1688e90b22dedc829f5abc9a912f18c034fbc 
.


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

https://reviews.llvm.org/D123065

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

[PATCH] D123065: [clang-tidy] support --load in clang-tidy-diff.py/run-clang-tidy.py

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D123065#3474107 , @bernhardmgruber 
wrote:

> Ping.
>
> Can someone merge this please for me? Thank you!

Yes, sure, just please specify what `user.name` and `user.email` would you like 
to be associated with the commit's author?


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

https://reviews.llvm.org/D123065

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

[PATCH] D123352: [analyzer] Add FixItHint to `nullability.NullReturnedFromNonnull` and `nullability.NullableReturnedFromNonnull`

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Regarding FixIts... FixIts are implemented in the "Diagnostic" library, which 
is non-specific to neither Clang-Tidy nor Sema whatsoever, they use the same 
infrastructure under the hood. Why the apply logic in CSA might do the same 
FixIt multiple times is beyond me, but I know that both 
`clang-apply-replacements` and `clang-tidy` go to length to ensure that in case 
multiple checkers report to the same location with potentially conflicting 
FixIts, then none gets applied, because applying all of them would result in 
ridiculously broken source code. They internally become an object in the 
`clang::tooling` namespace which is implemented as a core Clang library. The 
relevant entrypoint to this logic, at least in Clang-Tidy, should be this one: 
http://github.com/llvm/llvm-project/blob/8f9dd5e608c0ac201ab682ccc89ac3be2dfd0d29/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L115-L134

FixIts are very rudimentary under the hood. Essentially, they allow you to, in 
a file at a given position, delete `N` characters and insert `M` characters. 
Removal fixits only delete, insertion fixits only insert (duh), and just like 
in version control, a //modification// is a "removal" and an "insertion" 
grouped into one action...

If FixIts are so broken in CSA's infrastructure as @steakhal observed then it 
might very well be a good thing to hold off on them and fix the infrastructure 
first. Or at least remove the ability to auto-apply them. You don't **have to** 
apply the fixes, but you can always get the neat little green-coloured notes as 
to what could the user should insert.

In D123352#3463063 , @MosheBerman 
wrote:

> There's a DeadStore checker that does have some fixit code, which is where I 
> found the `FixItHint` class. I did notice it isn't used in too many other 
> places.



In D123352#3439390 , @steakhal wrote:

> For fixits, we should be confident that some property holds, but the static 
> analyzer might conclude erroneously that a given pointer is null resulting in 
> a **bad** fixit.

Dead stores and dead code are, for the most part, rather easy to reason about 
and not make dangerous fixits by their removal. (I'd go far enough to say the 
DeadStore checker should be a run-of-the-mill warning in Sema, and not some 
extra fluff only available in CSA...)

In D123352#3439649 , @MosheBerman 
wrote:

> Where can I learn more about this?  Would it be possible and 
> idiomatically/architecturally sounds to write a clang-tidy that processes 
> output from this checker?

Very unlikely. While Clang-Tidy can execute CSA checkers (because CSA is a 
library within Clang, but AFAIK calling CSA from Tidy doesn't handle things 
like CTU), Clang-Tidy does not contain **any** notion of dependency between 
checks and they are meant to be independent. Moreover, you would need to 
enforce a sequential execution by the user //across// binaries (not to mention 
the fact that the entire project needs to be parsed twice, which might not be a 
trivial cost!), which seems error-prone...

-

In general, why you are trying to achieve this with the static analyser? It 
looks to me as if what you want is to mechanically add an annotation to some 
types if they are in between specific macros / annotations. More specifically, 
the table that is in your original post doesn't seem to involve path-sensitive 
information. So could this be a purely (platform-specific?) Tidy check? Your 
test code also doesn't contain anything that seems to depend on path-sensitive 
information.

1. Where is this `NS_` annotation coming from? Is this a thing of a specific 
platform, or specific to your company's products?
2. If you are sure you will not depend on any path-sensitive symbolic execution 
information (which you currently appear you do not), and the answer to `1.` is 
meaningful enough to be useful for a wider community (that is, not just you or 
your company?), then I would suggest rewriting the whole thing within 
Clang-Tidy. Clang-Tidy works off of AST matchers directly. The nullability 
annotations are attributes, which are also AST nodes, so you can handle them 
similarly to matching types or Decls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

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

[PATCH] D118996: [clang-tidy] Support C++14 in bugprone-signal-handler.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

After adding improvements to the documentation, I think this will be good to 
go, and thank you! Perhaps just for a safety measure you could run it on a few 
projects (LLVM itself?) to ensure we didn't miss a case where it might 
magically crash, but I wonder how many specifically "C++14" projects will use 
signal handlers in the first place.




Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:328
 const LangOptions ) const {
-  // FIXME: Make the checker useful on C++ code.
-  if (LangOpts.CPlusPlus)
-return false;
-
-  return true;
+  return LangOpts.CPlusPlus17 == 0;
 }

Aren't these `bool`s?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118996

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

[PATCH] D123992: [clang-tidy] Fix crash on calls to overloaded operators in llvmlibc-callee-namespace

[Whisperity via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf4834815f439: [clang-tidy] Fix crash on calls to overloaded 
operators in `llvmlibc-callee… (authored by whisperity).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123992

Files:
  clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp
@@ -5,6 +5,10 @@
 void nested_func() {}
 } // namespace nested
 void libc_api_func() {}
+
+struct libc_api_struct {
+  int operator()() const { return 0; }
+};
 } // namespace __llvm_libc
 
 // Emulate a function from the public headers like string.h
@@ -13,6 +17,11 @@
 // Emulate a function specifically allowed by the exception list.
 void malloc() {}
 
+// Emulate a non-trivially named symbol.
+struct global_struct {
+  int operator()() const { return 0; }
+};
+
 namespace __llvm_libc {
 void Test() {
   // Allow calls with the fully qualified name.
@@ -30,19 +39,28 @@
   void (*barePtr)(void) = __llvm_libc::libc_api_func;
   barePtr();
 
+  // Allow calling entities defined in the namespace.
+  __llvm_libc::libc_api_struct{}();
+
   // Disallow calling into global namespace for implemented entrypoints.
   ::libc_api_func();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'libc_api_func' must resolve to a function declared within the '__llvm_libc' namespace
-  // CHECK-MESSAGES: :11:6: note: resolves to this declaration
+  // CHECK-MESSAGES: :15:6: note: resolves to this declaration
 
   // Disallow indirect references to functions in global namespace.
   void (*badPtr)(void) = ::libc_api_func;
   badPtr();
   // CHECK-MESSAGES: :[[@LINE-2]]:26: warning: 'libc_api_func' must resolve to a function declared within the '__llvm_libc' namespace
-  // CHECK-MESSAGES: :11:6: note: resolves to this declaration
+  // CHECK-MESSAGES: :15:6: note: resolves to this declaration
 
   // Allow calling into global namespace for specific functions.
   ::malloc();
+
+  // Disallow calling on entities that are not in the namespace, but make sure
+  // no crashes happen.
+  global_struct{}();
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'operator()' must resolve to a function declared within the '__llvm_libc' namespace
+  // CHECK-MESSAGES: :22:7: note: resolves to this declaration
 }
 
 } // namespace __llvm_libc
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,31 +136,39 @@
 Changes in existing checks
 ^^
 
-- Improved :doc:`performance-inefficient-vector-operation 
-  ` to work when
-  the vector is a member of a structure.
-
-- Fixed a false positive in :doc:`readability-non-const-parameter
-  ` when the parameter is referenced by an lvalue.
-
-- Fixed a crash in :doc:`readability-const-return-type
-  ` when a pure virtual function
-  overrided has a const return type. Removed the fix for a virtual function.
+- Fixed a crash in :doc:`bugprone-sizeof-expression
+  ` when `sizeof(...)` is
+  compared against a `__int128_t`.
 
-- Fixed a false positive in :doc:`misc-redundant-expression `
-  involving overloaded comparison operators.
-
-- Fixed a crash in :doc:`bugprone-sizeof-expression ` when
-  `sizeof(...)` is compared agains a `__int128_t`.
-  
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   ` check.
 
   Fixed an issue when there was already an initializer in the constructor and
   the check would try to create another initializer for the same member.
 
-- Fixed a false positive in :doc:`misc-redundant-expression `
-  involving assignments in conditions. This fixes `Issue 35853 `_.
+- Fixed a crash in :doc:`llvmlibc-callee-namespace
+  ` when executing for C++ code
+  that contain calls to advanced constructs, e.g. overloaded operators.
+
+- Fixed a false positive in :doc:`misc-redundant-expression
+  ` involving overloaded
+  comparison operators.
+
+- Fixed a false positive in :doc:`misc-redundant-expression
+  ` involving assignments in
+  conditions. This fixes `Issue 35853 `_.
+
+- Fixed a crash in :doc:`readability-const-return-type
+  ` when a pure virtual function
+  overrided has a const return type. Removed the fix for a virtual function.
+
+- Fixed a false positive in :doc:`readability-non-const-parameter
+  ` when the parameter is
+  referenced by an lvalue.
+
+- Improved 

[PATCH] D123992: [clang-tidy] Fix crash on calls to overloaded operators in llvmlibc-callee-namespace

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 423595.
whisperity added a comment.

**[NFC]** Fix the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123992

Files:
  clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp
@@ -5,6 +5,10 @@
 void nested_func() {}
 } // namespace nested
 void libc_api_func() {}
+
+struct libc_api_struct {
+  int operator()() const { return 0; }
+};
 } // namespace __llvm_libc
 
 // Emulate a function from the public headers like string.h
@@ -13,6 +17,11 @@
 // Emulate a function specifically allowed by the exception list.
 void malloc() {}
 
+// Emulate a non-trivially named symbol.
+struct global_struct {
+  int operator()() const { return 0; }
+};
+
 namespace __llvm_libc {
 void Test() {
   // Allow calls with the fully qualified name.
@@ -30,19 +39,28 @@
   void (*barePtr)(void) = __llvm_libc::libc_api_func;
   barePtr();
 
+  // Allow calling entities defined in the namespace.
+  __llvm_libc::libc_api_struct{}();
+
   // Disallow calling into global namespace for implemented entrypoints.
   ::libc_api_func();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'libc_api_func' must resolve to a function declared within the '__llvm_libc' namespace
-  // CHECK-MESSAGES: :11:6: note: resolves to this declaration
+  // CHECK-MESSAGES: :15:6: note: resolves to this declaration
 
   // Disallow indirect references to functions in global namespace.
   void (*badPtr)(void) = ::libc_api_func;
   badPtr();
   // CHECK-MESSAGES: :[[@LINE-2]]:26: warning: 'libc_api_func' must resolve to a function declared within the '__llvm_libc' namespace
-  // CHECK-MESSAGES: :11:6: note: resolves to this declaration
+  // CHECK-MESSAGES: :15:6: note: resolves to this declaration
 
   // Allow calling into global namespace for specific functions.
   ::malloc();
+
+  // Disallow calling on entities that are not in the namespace, but make sure
+  // no crashes happen.
+  global_struct{}();
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'operator()' must resolve to a function declared within the '__llvm_libc' namespace
+  // CHECK-MESSAGES: :22:7: note: resolves to this declaration
 }
 
 } // namespace __llvm_libc
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,31 +136,39 @@
 Changes in existing checks
 ^^
 
-- Improved :doc:`performance-inefficient-vector-operation 
-  ` to work when
-  the vector is a member of a structure.
-
-- Fixed a false positive in :doc:`readability-non-const-parameter
-  ` when the parameter is referenced by an lvalue.
-
-- Fixed a crash in :doc:`readability-const-return-type
-  ` when a pure virtual function
-  overrided has a const return type. Removed the fix for a virtual function.
+- Fixed a crash in :doc:`bugprone-sizeof-expression
+  ` when `sizeof(...)` is
+  compared against a `__int128_t`.
 
-- Fixed a false positive in :doc:`misc-redundant-expression `
-  involving overloaded comparison operators.
-
-- Fixed a crash in :doc:`bugprone-sizeof-expression ` when
-  `sizeof(...)` is compared agains a `__int128_t`.
-  
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   ` check.
 
   Fixed an issue when there was already an initializer in the constructor and
   the check would try to create another initializer for the same member.
 
-- Fixed a false positive in :doc:`misc-redundant-expression `
-  involving assignments in conditions. This fixes `Issue 35853 `_.
+- Fixed a crash in :doc:`llvmlibc-callee-namespace
+  ` when executing for C++ code
+  that contain calls to advanced constructs, e.g. overloaded operators.
+
+- Fixed a false positive in :doc:`misc-redundant-expression
+  ` involving overloaded
+  comparison operators.
+
+- Fixed a false positive in :doc:`misc-redundant-expression
+  ` involving assignments in
+  conditions. This fixes `Issue 35853 `_.
+
+- Fixed a crash in :doc:`readability-const-return-type
+  ` when a pure virtual function
+  overrided has a const return type. Removed the fix for a virtual function.
+
+- Fixed a false positive in :doc:`readability-non-const-parameter
+  ` when the parameter is
+  referenced by an lvalue.
+
+- Improved :doc:`performance-inefficient-vector-operation
+  ` to work when
+  the vector is a member of a structure.
 
 

[PATCH] D114292: [clang-tidy] Fix `altera-struct-pack-align` check for empty structs

[Whisperity via Phabricator via cfe-commits]

whisperity accepted this revision.
whisperity added a comment.
This revision is now accepted and ready to land.
Herald added a project: All.

@aaron.ballman Alright, I think this can go. The `ReleaseNotes.rst` needs a 
rebase anyway.


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

https://reviews.llvm.org/D114292

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

[PATCH] D123992: [clang-tidy] Fix crash on calls to overloaded operators in llvmlibc-callee-namespace

[Whisperity via Phabricator via cfe-commits]

whisperity created this revision.
whisperity added reviewers: aaron.ballman, njames93, PaulkaToast.
whisperity added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, martong, gamesh411, Szelethus, dkrupp, 
rnkovacs, xazax.hun.
Herald added a project: All.
whisperity requested review of this revision.
Herald added a subscriber: cfe-commits.

The routine that facilitated symbols to be explicitly allowed asked the name of 
the called function, which resulted in a crash when the check was accidentally 
run on non-trivial C++ code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123992

Files:
  clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp
@@ -5,6 +5,10 @@
 void nested_func() {}
 } // namespace nested
 void libc_api_func() {}
+
+struct libc_api_struct {
+  int operator()() const { return 0; }
+};
 } // namespace __llvm_libc
 
 // Emulate a function from the public headers like string.h
@@ -13,6 +17,11 @@
 // Emulate a function specifically allowed by the exception list.
 void malloc() {}
 
+// Emulate a non-trivially named symbol.
+struct global_struct {
+  int operator()() const { return 0; }
+};
+
 namespace __llvm_libc {
 void Test() {
   // Allow calls with the fully qualified name.
@@ -30,6 +39,9 @@
   void (*barePtr)(void) = __llvm_libc::libc_api_func;
   barePtr();
 
+  // Allow calling entities defined in the namespace.
+  __llvm_libc::libc_api_struct{}();
+
   // Disallow calling into global namespace for implemented entrypoints.
   ::libc_api_func();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'libc_api_func' must resolve to a function declared within the '__llvm_libc' namespace
@@ -43,6 +55,12 @@
 
   // Allow calling into global namespace for specific functions.
   ::malloc();
+
+  // Disallow calling on entities that are not in the namespace, but make sure
+  // no crashes happen.
+  global_struct{}();
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'operator()' must resolve to a function declared within the '__llvm_libc' namespace
+  // CHECK-MESSAGES: :22:7: note: resolves to this declaration
 }
 
 } // namespace __llvm_libc
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,31 +136,39 @@
 Changes in existing checks
 ^^
 
-- Improved :doc:`performance-inefficient-vector-operation 
-  ` to work when
-  the vector is a member of a structure.
-
-- Fixed a false positive in :doc:`readability-non-const-parameter
-  ` when the parameter is referenced by an lvalue.
-
-- Fixed a crash in :doc:`readability-const-return-type
-  ` when a pure virtual function
-  overrided has a const return type. Removed the fix for a virtual function.
+- Fixed a crash in :doc:`bugprone-sizeof-expression
+  ` when `sizeof(...)` is
+  compared against a `__int128_t`.
 
-- Fixed a false positive in :doc:`misc-redundant-expression `
-  involving overloaded comparison operators.
-
-- Fixed a crash in :doc:`bugprone-sizeof-expression ` when
-  `sizeof(...)` is compared agains a `__int128_t`.
-  
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   ` check.
 
   Fixed an issue when there was already an initializer in the constructor and
   the check would try to create another initializer for the same member.
 
-- Fixed a false positive in :doc:`misc-redundant-expression `
-  involving assignments in conditions. This fixes `Issue 35853 `_.
+- Fixed a crash in :doc:`llvmlibc-callee-namespace
+  ` when executing for C++ code
+  that contain calls to advanced constructs, e.g. overloaded operators.
+
+- Fixed a false positive in :doc:`misc-redundant-expression
+  ` involving overloaded
+  comparison operators.
+
+- Fixed a false positive in :doc:`misc-redundant-expression
+  ` involving assignments in
+  conditions. This fixes `Issue 35853 `_.
+
+- Fixed a crash in :doc:`readability-const-return-type
+  ` when a pure virtual function
+  overrided has a const return type. Removed the fix for a virtual function.
+
+- Fixed a false positive in :doc:`readability-non-const-parameter
+  ` when the parameter is
+  referenced by an lvalue.
+
+- Improved :doc:`performance-inefficient-vector-operation
+  ` to work when
+  the vector is a member of a structure.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp

[PATCH] D121387: [analyzer] ClangSA should tablegen doc urls refering to the main doc page

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

(Side note: you should avoid the list-expansion syntax in URLs because browsers 
do not understand them and result in links that are not leading anywhere.)

I like this. We should continue getting to the point where the documentation is 
having a uniform layout.


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

https://reviews.llvm.org/D121387

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

[PATCH] D118996: [clang-tidy] Support C++14 in bugprone-signal-handler.

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:153
+diag(HandlerLambda->getBeginLoc(),
+ "lambda function is not allowed as signal handler (until C++17)")
+<< HandlerLambda->getSourceRange();

balazske wrote:
> whisperity wrote:
> > I am trying to find some source for this claim but so far hit a blank.  
> > Could you please elaborate where this information comes from? Most notably, 
> > [[ https://en.cppreference.com/w/cpp/utility/program/signal | std::signal 
> > on CppReference ]] makes no mention of this, which would be the first place 
> > I would expect most people to look at.
> > 
> > Maybe this claim is derived from the rule that signal handlers **MUST** 
> > have C linkage? (If so, is there a warning against people setting 
> > C++-linkage functions as signal handlers in this check in general?)
> This check is made to comply with a CERT rule [[ 
> https://wiki.sei.cmu.edu/confluence/display/cplusplus/MSC54-CPP.+A+signal+handler+must+be+a+plain+old+function
>  | MSC54-CPP ]]. According to this only the subset of C and C++ should be 
> used in a signal handler, and it should have extern "C" linkage. This does 
> not allow lambda functions because the linkage restriction (if not others). 
> (From C++17 on other rules apply, this check is not applicable for such code.)
Ah, right, I found it a bit further down the page:

> //Signal handlers are expected to have C linkage// and, in general, only use 
> the features from the common subset of C and C++. It is 
> implementation-defined if a function with C++ linkage can be used as a signal 
> handler. (until C++17)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118996

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

[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Message improvement and code refactoring.

[Whisperity via Phabricator via cfe-commits]

whisperity accepted this revision.
whisperity added a comment.
This revision is now accepted and ready to land.

Alright, I think this is good to go. I like that it makes it clear that the 
called function is coming from some external source (system header or 
otherwise).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118370

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

[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Message improvement and code refactoring.

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:159-160
Itr != ItrE; ++Itr) {
 const auto *CallF = dyn_cast((*Itr)->getDecl());
-if (CallF && !isFunctionAsyncSafe(CallF)) {
-  assert(Itr.getPathLength() >= 2);
-  reportBug(CallF, findCallExpr(Itr.getPath(Itr.getPathLength() - 2), 
*Itr),
-/*DirectHandler=*/false);
-  reportHandlerCommon(Itr, SignalCall, HandlerDecl, HandlerExpr);
+if (CallF) {
+  unsigned int PathL = Itr.getPathLength();

balazske wrote:
> whisperity wrote:
> > 
> Is this code not too long to put in the condition section?
The formatter will break it up appropriately, no problem with that.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c:14
   printf("1234");
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be 
asynchronous-safe; calling it from a signal handler may be dangerous 
[bugprone-signal-handler]
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call 'printf' may not be 
asynchronous-safe; calling it from a signal handler may be dangerous 
[bugprone-signal-handler]
 }

balazske wrote:
> whisperity wrote:
> > I'm not exactly sure we should call `printf` (and `memcpy`) a "system 
> > call". As far as I can see, in this patch, `isSystemCall()` boils down to 
> > //"declaration in file included as system header"//
> Call it "standard function"?
Yes, that would be better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118370

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

[PATCH] D121214: [clang-tidy][docs][NFC] Add alias cert-mem51-cpp to bugprone-shared-ptr-array-mismatch

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D121214#3369871 , @steakhal wrote:

> Drop the alias-related changes and preserve the note in the 
> `bugprone-shared-ptr-array-mismatch.rst` stating this relationship with the 
> cert rule?
> If we do it like that, then this will be again NFC.

I would suggest definitely doing that. Perhaps with a bit more emphasis on this 
one catching the `shared_ptr` case only.
If you could create the check for the `unique_ptr` case, that would also be 
great.
I think some of MEM51-CPP is covered by the Static Analyser, right? (Things 
like `free()`ing a `new`-created pointer.)

In D121214#3369871 , @steakhal wrote:

> How shall I proceed?

Let's ask @aaron.ballman or @njames93 with regards to that. The long-term 
solution would be implementing the `1-to-N` check aliasing in the 
infrastructure, but I understand it might be an out-of-scope work right now.



However, I am also in favour of creating a mapping of "partially implemented 
guidelines" in the documentation somewhere... maybe in the `list.rst`, maybe on 
a separate page. There, we could start documenting cases like this one... It's 
less direct than an alias, but more accessible to users who wish to cover as 
many rules of thumb as possible than having to iterate through hundreds of 
distinct documentation pages for the checks and trying to deduce //"Hey, so 
this one makes me catch some of that guideline!"//.

Consider (in a Markdown example, because I can never remember at the top of my 
head how the heading level magic characters of RST are in sequence...)

  # Guideline coverage
  
  ## CppCoreGuidelines
  
  {first the fully covered, I believe the cross-reference links here are enough}
   * X.00 Blah blah rule | cppcoreguidelines-foo-bar
   * Y.01 Blah blah rule | cppcoreguidelines-baz-qux
  
  {then the partially covered}
   * Z.99 Blah blah rule | misc-whatever-something, bugprone-fancy-diagnostics
  
  ## CERT
  <... same format ...>
  
  <... etc. ...>

Note: I'm not saying you should go ahead and create the ENTIRE collection of 
these data right now, just to start pinning down a format. A //"work in 
progress"// marker could also be added to the top of this new page.


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

https://reviews.llvm.org/D121214

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

[PATCH] D121214: [clang-tidy][docs][NFC] Add alias cert-mem51-cpp to bugprone-shared-ptr-array-mismatch

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

I have some concerns about this. While it is now clear to me that the 
//partial//ness refers to partial coverage of the guideline rule, it is indeed 
very, very partial. **MEM51-CPP** as of now lists **9** cases of non-compliant 
examples, from which `std::shared_ptr = new S[8];` is just //one//. 
`bugprone-shared-ptr-array-mismatch` (D117306 
) in itself is a check that inherits from a 
"more generic" smart pointer check.

Right now, there is no check for the exact same `unique_ptr` case, which the 
linked CERT site explicitly phrases:

> As with std::unique_ptr, when the std::shared_ptr is destroyed [...]

Let's suppose a `bugprone-unique-ptr-array-mismatch` check is also created 
(even though that would mean immediately that the naming of the `shared_ptr` 
one is kind of bad and that we should rename or deprecate the check... which is 
its own can of worms with regards to support of clients...). What will this 
alias become, then?

At first re-read my recent comment of

In D121214#3367609 , @whisperity 
wrote:

> Is "partial aliasing" a new notion?

sounded weird to me too, but thinking this through again, I think I know what I 
was thinking yesterday.

Please correct me if I'm wrong, but the "name" of a check (alias or not) is a 
**unique** property. We lack the infrastructure support for `1:N` aliasing. 
While there are certainly cases where we only managed to partially cover a 
rule, and made an alias for it, missing a small chunk of the guideline (e.g. 
because static analyses can't catch such) looks less intrusive than -- 
essentially -- //"trying to sell 1/9 coverage as **the** alias"//.




Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-mem51-cpp.rst:12
+
+Please note that the check only partially covers the corresponding CERT rule.

//Maybe// this warning could be emphasised with the appropriate RST entity, but 
a quick grep of Tidy docs turned up no such similar constructs so maybe it is 
better if we do not break form here...




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

https://reviews.llvm.org/D121214

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

[PATCH] D121214: [clang-tidy][docs][NFC] Add alias cert-mem51-cpp to bugprone-shared-ptr-array-mismatch

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Is "partial aliasing" a new notion? Or... is the alias not partial, but the 
coverage?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121214

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

[PATCH] D118996: [clang-tidy] Support C++14 in bugprone-signal-handler.

[Whisperity via Phabricator via cfe-commits]

whisperity added a reviewer: whisperity.
whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:142-149
   Finder->addMatcher(
   callExpr(callee(SignalFunction), hasArgument(1, HandlerExpr))
   .bind("register_call"),
   this);
+  Finder->addMatcher(
+  callExpr(callee(SignalFunction), hasArgument(1, HandlerLambda))
+  .bind("register_call"),

LegalizeAdulthood wrote:
> Is there any utility/performance to be gained by combining these into a 
> single matcher?
> e.g.
> ```
> callExpr(callee(SignalFunction), anyOf(hasArgument(1, HandlerExpr), 
> hasArgument(1, HandlerLambda)))
> ```
> (not sure if that is syntactically valid, but you get my point)
Or perhaps

```
callExpr(callee(SignalFunction), hasArgument(1, expr(anyOf(HandlerExpr, 
HandlerLambda;
```



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:247
 
+  if (!getLangOpts().CPlusPlus17 && getLangOpts().CPlusPlus)
+return checkFunctionCPP14(FD, CallOrRef, ChainReporter);

balazske wrote:
> LegalizeAdulthood wrote:
> > How can the first expression here ever be false when
> > we rejected C++17 in the `isLanguageVersionSupported`
> > override?
> I was thinking for the case when this check supports C++17 too. The change 
> can be added later, this makes current code more readable.
(True. I think @LegalizeAdulthood's comment can be marked as Done.)



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:41
 
+bool isInGlobalNamespace(const FunctionDecl *FD) {
+  const DeclContext *DC = FD->getDeclContext();

Is this feature not available in (some parent class of) `FunctionDecl`?



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:46
+  assert(DC && "Function should have a declaration context.");
+  return DC->isTranslationUnit();
+}

(Perhaps a >= C++20 fixme that this query here //might// not handle modules 
well?)



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:94
 
+SourceRange getSourceRangeOfStmt(const Stmt *S, ASTContext ) {
+  ParentMapContext  = Ctx.getParentMapContext();

Okay, this is interesting... Why is `Ctx` not `const`? Unless 
**`get`**`Something()` is changing things (which is non-intuitive then...), as 
far as I can tell, you're only reading from `Ctx`.



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:96
+  ParentMapContext  = Ctx.getParentMapContext();
+  DynTypedNode P = DynTypedNode::create(*S);
+  while (P.getSourceRange().getBegin().isInvalid()) {

Are these objects cleaned up properly in their destructor (same question for 
`DynTypeNodeList`)?



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:97
+  DynTypedNode P = DynTypedNode::create(*S);
+  while (P.getSourceRange().getBegin().isInvalid()) {
+DynTypedNodeList PL = PM.getParents(P);

`SourceRange` itself should have a sentinel query. Why is only the beginning of 
it interesting?



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:99-101
+if (PL.size() != 1)
+  return {};
+P = PL[0];

`size != 1` could still mean `size == 0` in which case taking an element seems 
dangerous. Is triggering such UB possible here?



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:153
+diag(HandlerLambda->getBeginLoc(),
+ "lambda function is not allowed as signal handler (until C++17)")
+<< HandlerLambda->getSourceRange();

I am trying to find some source for this claim but so far hit a blank.  Could 
you please elaborate where this information comes from? Most notably, [[ 
https://en.cppreference.com/w/cpp/utility/program/signal | std::signal on 
CppReference ]] makes no mention of this, which would be the first place I 
would expect most people to look at.

Maybe this claim is derived from the rule that signal handlers **MUST** have C 
linkage? (If so, is there a warning against people setting C++-linkage 
functions as signal handlers in this check in general?)



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:162
   const auto *HandlerExpr = 
Result.Nodes.getNodeAs("handler_expr");
+
   assert(SignalCall && HandlerDecl && HandlerExpr &&

(Nit: maybe it is better without this newline, to visibly demarcate the "local 
state init "block"" of the function.)



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:178-179
+// Check the handler function.
+// The warning is placed to the signal handler registration.
+// No need to display a call chain and no need for more checks.
+

[PATCH] D120555: [clang-tidy] Fix `readability-suspicious-call-argument` crash for arguments without name-like identifier

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D120555#3345707 , @njames93 wrote:

> If you backport, the release notes change on trunk should then be reverted.

Release Notes entry of current development tree reverted in 
rG94850918274c20c15c6071dc52314df51ed2a357 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120555

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

[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Message improvement and code refactoring.

[Whisperity via Phabricator via cfe-commits]

whisperity added a reviewer: whisperity.
whisperity added a comment.
Herald added a subscriber: rnkovacs.

This generally looks good, and thank you! I have a few minor comments (mostly 
presentation and documentation).




Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:72-73
 
 /// Given a call graph node of a function and another one that is called from
 /// this function, get a CallExpr of the corresponding function call.
 /// It is unspecified which call is found if multiple calls exist, but the 
order

(Nit: There are a lot of indirections in the documentation that did not help 
understanding what is happening here.)



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:159-160
Itr != ItrE; ++Itr) {
 const auto *CallF = dyn_cast((*Itr)->getDecl());
-if (CallF && !isFunctionAsyncSafe(CallF)) {
-  assert(Itr.getPathLength() >= 2);
-  reportBug(CallF, findCallExpr(Itr.getPath(Itr.getPathLength() - 2), 
*Itr),
-/*DirectHandler=*/false);
-  reportHandlerCommon(Itr, SignalCall, HandlerDecl, HandlerExpr);
+if (CallF) {
+  unsigned int PathL = Itr.getPathLength();





Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:165-171
+  if (checkFunction(CallF, CallOrRef))
+reportHandlerChain(Itr, HandlerExpr);
 }
   }
 }
 
+bool SignalHandlerCheck::checkFunction(const FunctionDecl *FD,

What does the return value of `checkFunction` signal to the user, and the `if`?



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:188
+  if (!FD->hasBody()) {
+diag(CallOrRef->getBeginLoc(), "cannot verify if external function %0 is "
+   "asynchronous-safe; "





Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:239
 // 
https://wiki.sei.cmu.edu/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
-llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
+const llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
 "signal", "abort", "_Exit", "quick_exit"};

whisperity wrote:
> Perchance this could work with `constexpr`? Although I'm not sure how widely 
> supported that is, given that we're pegged at C++14 still.
Otherwise, **if** these are truly const and we are reasonably sure they can be 
instantiated on any meaningful system without crashing, then these could be a 
TU-`static`. It's still iffy, but that technique is used in many places already.



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:239-240
 // 
https://wiki.sei.cmu.edu/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
-llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
+const llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
 "signal", "abort", "_Exit", "quick_exit"};
 

Perchance this could work with `constexpr`? Although I'm not sure how widely 
supported that is, given that we're pegged at C++14 still.



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:48-49
   bool isSystemCallAsyncSafe(const FunctionDecl *FD) const;
-  void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef,
- bool DirectHandler);
-  void reportHandlerCommon(llvm::df_iterator Itr,
-   const CallExpr *SignalCall,
-   const FunctionDecl *HandlerDecl,
-   const Expr *HandlerRef);
+  /// Add bug report notes to show the call chain of functions from a signal
+  /// handler to an actual called function (from it).
+  /// @param Itr Position during a call graph depth-first iteration. It 
contains

First, we usually call these "diagnostics" in Tidy and not "bug report notes", 
which to me seems like CSA terminology. Second... It's not clear to me what 
`(from it)` is referring to. //"[...] show call chain from a signal handler to 
a [... specific? provided? expected? ...] function"// sounds alright to me.



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:54
+  /// @param HandlerRef Reference to the signal handler function where it is
+  /// registered (considered as first part of the call chain).
+  void reportHandlerChain(const llvm::df_iterator ,





Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:46-49
   llvm::StringSet<> 
 
   static llvm::StringSet<> MinimalConformingFunctions;
   static llvm::StringSet<> POSIXConformingFunctions;

njames93 wrote:
> While you're refactoring, those static StringSets really belong in the 
> implementation file, and the 

[PATCH] D120555: [clang-tidy] Fix `readability-suspicious-call-argument` crash for arguments without name-like identifier

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D120555#3345707 , @njames93 wrote:

> If you backport, the release notes change on trunk should then be reverted.

Indeed, this is not the first time I mess this up... Ugh.
I'll wait a little bit for that to make sure the backport is stable, and then I 
will revert that part of the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120555

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

[PATCH] D120555: [clang-tidy] Fix `readability-suspicious-call-argument` crash for arguments without name-like identifier

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Thanks! I'll re-run the tests on top of **14.0** and do the backport too, soon. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120555

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

[PATCH] D120555: [clang-tidy] Fix `readability-suspicious-call-argument` crash for arguments without name-like identifier

[Whisperity via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG416e689ecda6: [clang-tidy] Fix 
`readability-suspicious-call-argument` crash for arguments… (authored by 
whisperity).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120555

Files:
  clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
@@ -485,3 +485,32 @@
 
   return 0;
 }
+
+namespace Issue_54074 {
+
+class T {};
+using OperatorTy = int(const T &, const T &);
+int operator-(const T &, const T &);
+
+template 
+struct Wrap {
+  Wrap(U);
+};
+
+template 
+void wrapTaker(V, Wrap);
+
+template 
+void wrapTaker(V a, V b, Wrap);
+
+void test() {
+  wrapTaker(0, operator-);
+  // NO-WARN. No crash!
+
+  int a = 4, b = 8;
+  wrapTaker(b, a, operator-);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'b' (passed to 
'a') looks like it might be swapped with the 2nd, 'a' (passed to 
'b')
+  // CHECK-MESSAGES: :[[@LINE-9]]:6: note: in the call to 'wrapTaker', 
declared here
+}
+
+} // namespace Issue_54074
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -112,6 +112,9 @@
 - Fixed a false positive in :doc:`readability-non-const-parameter
   ` when the parameter is 
referenced by an lvalue
 
+- Fixed a crash in :doc:`readability-suspicious-call-argument
+  ` related to passing
+  arguments that refer to program elements without a trivial identifier.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
@@ -711,23 +711,28 @@
 
   for (std::size_t I = InitialArgIndex, J = MatchedCallExpr->getNumArgs();
I < J; ++I) {
+assert(ArgTypes.size() == I - InitialArgIndex &&
+   ArgNames.size() == ArgTypes.size() &&
+   "Every iteration must put an element into the vectors!");
+
 if (const auto *ArgExpr = dyn_cast(
 MatchedCallExpr->getArg(I)->IgnoreUnlessSpelledInSource())) {
   if (const auto *Var = dyn_cast(ArgExpr->getDecl())) {
 ArgTypes.push_back(Var->getType());
 ArgNames.push_back(Var->getName());
-  } else if (const auto *FCall =
- dyn_cast(ArgExpr->getDecl())) {
-ArgTypes.push_back(FCall->getType());
-ArgNames.push_back(FCall->getName());
-  } else {
-ArgTypes.push_back(QualType());
-ArgNames.push_back(StringRef());
+continue;
+  }
+  if (const auto *FCall = dyn_cast(ArgExpr->getDecl())) {
+if (FCall->getNameInfo().getName().isIdentifier()) {
+  ArgTypes.push_back(FCall->getType());
+  ArgNames.push_back(FCall->getName());
+  continue;
+}
   }
-} else {
-  ArgTypes.push_back(QualType());
-  ArgNames.push_back(StringRef());
 }
+
+ArgTypes.push_back(QualType());
+ArgNames.push_back(StringRef());
   }
 }
 


Index: clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
@@ -485,3 +485,32 @@
 
   return 0;
 }
+
+namespace Issue_54074 {
+
+class T {};
+using OperatorTy = int(const T &, const T &);
+int operator-(const T &, const T &);
+
+template 
+struct Wrap {
+  Wrap(U);
+};
+
+template 
+void wrapTaker(V, Wrap);
+
+template 
+void wrapTaker(V a, V b, Wrap);
+
+void test() {
+  wrapTaker(0, operator-);
+  // NO-WARN. No crash!
+
+  int a = 4, b = 8;
+  wrapTaker(b, a, operator-);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'b' (passed to 'a') looks like it might be swapped with the 2nd, 'a' (passed to 'b')
+  // CHECK-MESSAGES: :[[@LINE-9]]:6: note: in the call to 'wrapTaker', declared here
+}
+
+} // namespace Issue_54074
Index: clang-tools-extra/docs/ReleaseNotes.rst

[PATCH] D120555: [clang-tidy] Fix `readability-suspicious-call-argument` crash for arguments without name-like identifier

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 411413.
whisperity added a comment.

Added //Release notes// entry.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120555

Files:
  clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
@@ -485,3 +485,32 @@
 
   return 0;
 }
+
+namespace Issue_54074 {
+
+class T {};
+using OperatorTy = int(const T &, const T &);
+int operator-(const T &, const T &);
+
+template 
+struct Wrap {
+  Wrap(U);
+};
+
+template 
+void wrapTaker(V, Wrap);
+
+template 
+void wrapTaker(V a, V b, Wrap);
+
+void test() {
+  wrapTaker(0, operator-);
+  // NO-WARN. No crash!
+
+  int a = 4, b = 8;
+  wrapTaker(b, a, operator-);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'b' (passed to 
'a') looks like it might be swapped with the 2nd, 'a' (passed to 
'b')
+  // CHECK-MESSAGES: :[[@LINE-9]]:6: note: in the call to 'wrapTaker', 
declared here
+}
+
+} // namespace Issue_54074
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -112,6 +112,9 @@
 - Fixed a false positive in :doc:`readability-non-const-parameter
   ` when the parameter is 
referenced by an lvalue
 
+- Fixed a crash in :doc:`readability-suspicious-call-argument
+  ` related to passing
+  arguments that refer to program elements without a trivial identifier.
 
 Removed checks
 ^^
Index: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
@@ -711,23 +711,28 @@
 
   for (std::size_t I = InitialArgIndex, J = MatchedCallExpr->getNumArgs();
I < J; ++I) {
+assert(ArgTypes.size() == I - InitialArgIndex &&
+   ArgNames.size() == ArgTypes.size() &&
+   "Every iteration must put an element into the vectors!");
+
 if (const auto *ArgExpr = dyn_cast(
 MatchedCallExpr->getArg(I)->IgnoreUnlessSpelledInSource())) {
   if (const auto *Var = dyn_cast(ArgExpr->getDecl())) {
 ArgTypes.push_back(Var->getType());
 ArgNames.push_back(Var->getName());
-  } else if (const auto *FCall =
- dyn_cast(ArgExpr->getDecl())) {
-ArgTypes.push_back(FCall->getType());
-ArgNames.push_back(FCall->getName());
-  } else {
-ArgTypes.push_back(QualType());
-ArgNames.push_back(StringRef());
+continue;
+  }
+  if (const auto *FCall = dyn_cast(ArgExpr->getDecl())) {
+if (FCall->getNameInfo().getName().isIdentifier()) {
+  ArgTypes.push_back(FCall->getType());
+  ArgNames.push_back(FCall->getName());
+  continue;
+}
   }
-} else {
-  ArgTypes.push_back(QualType());
-  ArgNames.push_back(StringRef());
 }
+
+ArgTypes.push_back(QualType());
+ArgNames.push_back(StringRef());
   }
 }
 


Index: clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
@@ -485,3 +485,32 @@
 
   return 0;
 }
+
+namespace Issue_54074 {
+
+class T {};
+using OperatorTy = int(const T &, const T &);
+int operator-(const T &, const T &);
+
+template 
+struct Wrap {
+  Wrap(U);
+};
+
+template 
+void wrapTaker(V, Wrap);
+
+template 
+void wrapTaker(V a, V b, Wrap);
+
+void test() {
+  wrapTaker(0, operator-);
+  // NO-WARN. No crash!
+
+  int a = 4, b = 8;
+  wrapTaker(b, a, operator-);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'b' (passed to 'a') looks like it might be swapped with the 2nd, 'a' (passed to 'b')
+  // CHECK-MESSAGES: :[[@LINE-9]]:6: note: in the call to 'wrapTaker', declared here
+}
+
+} // namespace Issue_54074
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -112,6 +112,9 @@
 - Fixed a false 

[PATCH] D120555: [clang-tidy] Fix `readability-suspicious-call-argument` crash for arguments without name-like identifier

[Whisperity via Phabricator via cfe-commits]

whisperity created this revision.
whisperity added reviewers: aaron.ballman, steakhal.
whisperity added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, martong, gamesh411, Szelethus, dkrupp, 
rnkovacs, xazax.hun.
whisperity requested review of this revision.
Herald added a subscriber: cfe-commits.

As originally reported by @steakhal in #54074 
, the name extraction logic 
in `readability-suspicious-call-argument` crashes if the argument passed to a 
function was a function call to a non-trivially named entity (e.g. an operator).

Fixed this crash case by ignoring such constructs and considering them as 
having no name.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120555

Files:
  clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
@@ -485,3 +485,32 @@
 
   return 0;
 }
+
+namespace Issue_54074 {
+
+class T {};
+using OperatorTy = int(const T &, const T &);
+int operator-(const T &, const T &);
+
+template 
+struct Wrap {
+  Wrap(U);
+};
+
+template 
+void wrapTaker(V, Wrap);
+
+template 
+void wrapTaker(V a, V b, Wrap);
+
+void test() {
+  wrapTaker(0, operator-);
+  // NO-WARN. No crash!
+
+  int a = 4, b = 8;
+  wrapTaker(b, a, operator-);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'b' (passed to 
'a') looks like it might be swapped with the 2nd, 'a' (passed to 
'b')
+  // CHECK-MESSAGES: :[[@LINE-9]]:6: note: in the call to 'wrapTaker', 
declared here
+}
+
+} // namespace Issue_54074
Index: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
@@ -711,23 +711,28 @@
 
   for (std::size_t I = InitialArgIndex, J = MatchedCallExpr->getNumArgs();
I < J; ++I) {
+assert(ArgTypes.size() == I - InitialArgIndex &&
+   ArgNames.size() == ArgTypes.size() &&
+   "Every iteration must put an element into the vectors!");
+
 if (const auto *ArgExpr = dyn_cast(
 MatchedCallExpr->getArg(I)->IgnoreUnlessSpelledInSource())) {
   if (const auto *Var = dyn_cast(ArgExpr->getDecl())) {
 ArgTypes.push_back(Var->getType());
 ArgNames.push_back(Var->getName());
-  } else if (const auto *FCall =
- dyn_cast(ArgExpr->getDecl())) {
-ArgTypes.push_back(FCall->getType());
-ArgNames.push_back(FCall->getName());
-  } else {
-ArgTypes.push_back(QualType());
-ArgNames.push_back(StringRef());
+continue;
+  }
+  if (const auto *FCall = dyn_cast(ArgExpr->getDecl())) {
+if (FCall->getNameInfo().getName().isIdentifier()) {
+  ArgTypes.push_back(FCall->getType());
+  ArgNames.push_back(FCall->getName());
+  continue;
+}
   }
-} else {
-  ArgTypes.push_back(QualType());
-  ArgNames.push_back(StringRef());
 }
+
+ArgTypes.push_back(QualType());
+ArgNames.push_back(StringRef());
   }
 }
 


Index: clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
@@ -485,3 +485,32 @@
 
   return 0;
 }
+
+namespace Issue_54074 {
+
+class T {};
+using OperatorTy = int(const T &, const T &);
+int operator-(const T &, const T &);
+
+template 
+struct Wrap {
+  Wrap(U);
+};
+
+template 
+void wrapTaker(V, Wrap);
+
+template 
+void wrapTaker(V a, V b, Wrap);
+
+void test() {
+  wrapTaker(0, operator-);
+  // NO-WARN. No crash!
+
+  int a = 4, b = 8;
+  wrapTaker(b, a, operator-);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 1st argument 'b' (passed to 'a') looks like it might be swapped with the 2nd, 'a' (passed to 'b')
+  // CHECK-MESSAGES: :[[@LINE-9]]:6: note: in the call to 'wrapTaker', declared here
+}
+
+} // namespace Issue_54074
Index: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
+++ 

[PATCH] D91000: [clang-tidy] Add cert-msc24-c checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a reviewer: whisperity.
whisperity added a comment.
Herald added a subscriber: rnkovacs.

Bump! Thanks @futogergely for picking up @ktomi996's work, and @steakhal for 
the help given during the implementation process!

In D91000#3231417 , @martong wrote:

> In D91000#3230055 , @futogergely 
> wrote:
>
>> I think for now it is enough to issue a warning of using these functions, 
>> and not suggest a replacement. Should we add an option to the checker to 
>> also check for these functions?
>
> IMHO, it is okay to start with just simply issuing the warning. Later we 
> might add that option (in a subsequent patch).

Yes, please. And I suggest indeed hiding it behind an option, e.g. 
//`ReportMoreUnsafeFunctions`// which I believe should default to true, but 
setting it to false would allow people who want their code to be "only" strict 
CERT-clean (that is, not care about these additional matches), can request 
doing so. Not offering a replacement right now is okay too, we can definitely 
work them in later.

In D91000#3197851 , @aaron.ballman 
wrote:

> In terms of whether we should expose the check in C++: I think we shouldn't. 
> Annex K *could* be supported by a C++ library implementation 
> (http://eel.is/c++draft/library#headers-10), but none of the identifiers are 
> added to namespace `std` and there's not a lot of Annex K support in C so I 
> imagine there's even less support in C++. We can always revisit the decision 
> later and expand support for C++ once we know what that support should look 
> like.

(Minor suggestion, but we could pull a trick with perhaps checking whether the 
macro is defined in the TU by the user and the library, and trying to match 
against the non-`std::` Annex K functions, and if they are available, consider 
the implementation the user is running under as 
"AnnexK-capable-even-in-C++-mode" and start issuing warnings? This might sound 
very technical, and definitely for later consideration!)

> I think we should probably also not enable the check when the user compiles 
> in C99 or earlier mode, because there is no Annex K available to provide 
> replacement functions.

Except for warning about `gets()` and suggesting `fgets()`?



Some nits: when it comes to inline comments, please mark the comments of the 
reviewers as "Done" when replying if you consider that comment was handled. 
Only you, the patch author, can do this. When there are too many open threads 
of comments, it gets messy to see what has been handled and what is still under 
discussion.




Comment at: 
clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:41-42
+
+  // Matching the `gets` deprecated function without replacement.
+  auto DeprecatedFunctionNamesMatcher = hasAnyName("::gets");
+

futogergely wrote:
> aaron.ballman wrote:
> > This comment is not accurate. `gets_s()` is a secure replacement for 
> > `gets()`.
> If gets is removed from C11, and gets_s is introduced in C11, then gets_s 
> cannot be a replacement or? Maybe fgets?
> 
> Also I was wondering if we would like to disable this check for C99, maybe we 
> should remove the check for gets all together.
Yes, it's strange territory. If I make my code safer but //stay// pre-C11, I 
actually can't, because the new function isn't there yet. If I also //upgrade// 
then I'll **have to** make my code "safer", because the old function is now 
missing...

Given how brutally dangerous `gets` is (you very rarely see a documentation 
page just going **`Never use gets()!`**), I would suggest keeping at least the 
warning.

Although even the CERT rule mentions `gets_s()`. We could have some wiggle room 
here: do the warning for `gets()`, and suggest two alternatives: `fgets()`, or 
`gets_s()` + Annex K? `fgets(stdin, ...)` is also safe, if the buffer's size is 
given appropriately.



Comment at: 
clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:41-42
+
+  // Matching the `gets` deprecated function without replacement.
+  auto DeprecatedFunctionNamesMatcher = hasAnyName("::gets");
+

whisperity wrote:
> futogergely wrote:
> > aaron.ballman wrote:
> > > This comment is not accurate. `gets_s()` is a secure replacement for 
> > > `gets()`.
> > If gets is removed from C11, and gets_s is introduced in C11, then gets_s 
> > cannot be a replacement or? Maybe fgets?
> > 
> > Also I was wondering if we would like to disable this check for C99, maybe 
> > we should remove the check for gets all together.
> Yes, it's strange territory. If I make my code safer but //stay// pre-C11, I 
> actually can't, because the new function isn't there yet. If I also 
> //upgrade// then I'll **have to** make my code "safer", because the old 
> function is now missing...
> 
> Given how brutally dangerous `gets` is (you very rarely see a documentation 

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Whisperity via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3696c70e67d9: [clang-tidy] Add 
`readability-container-contains` check (authored by avogelsgesang, committed by 
whisperity).

Changed prior to commit:
  https://reviews.llvm.org/D112646?vs=400820=402464#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,230 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+  void *find(const Key );
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+} // namespace std
+
+// Check that we detect various common ways to check for membership
+int testDifferentCheckTypes(std::map ) {
+  if (MyMap.count(0))
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: if (MyMap.contains(0))
+return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(1);
+  auto C2 = static_cast(MyMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1));
+  auto C3 = MyMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = MyMap.contains(4);
+  auto C6 = MyMap.find(5) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C6 = MyMap.contains(5);
+  return C1 + C2 + C3 + C4 + C5 + C6;
+}
+
+// Check that we detect various common ways to check for non-membership
+int testNegativeChecks(std::map ) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = !MyMap.contains(-4);
+  auto C5 = MyMap.find(-5) == MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = !MyMap.contains(-5);
+  return C1 + C2 + C3 + C4 + C5;
+}
+
+// Check for various types
+int testDifferentTypes(std::map , std::unordered_set , std::set , std::multimap ) {
+  bool C1 = M.count(1001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = M.contains(1001);
+  bool C2 = US.count(1002);
+  // 

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Whisperity via Phabricator via cfe-commits]

whisperity accepted this revision.
whisperity added a comment.

In D112646#3265670 , @whisperity 
wrote:

> Please specify what name and e-mail address you'd like to have the Git commit 
> author attributed with!

Nevermind, I forgot that you commented this:

In D112646#3264010 , @avogelsgesang 
wrote:

> there is a copy of this commit on 
> https://github.com/vogelsgesang/llvm-project/commits/avogelsgesang-tidy-readability-container-contains





LGTM. Committing in a moment.  (I applied a fix to a typo in a comment in the 
test, `time begin` -> `time being`.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D112646#3251025 , @avogelsgesang 
wrote:

> @xazax.hun Can you please merge this to `main` on my behalf? (I don't have 
> commit rights)

Hey! Sorry for my absence, I'm terribly busy with doing stuffs  I'll check the 
patch now, generally looks good. Please specify what name and e-mail address 
you'd like to have the Git commit author attributed with!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D91000: [clang-tidy] Add cert-msc24-c checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Should/does this work in C++ mode for `std::whatever`?




Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:48
+  // Matching functions with safe replacements in annex K.
+  auto FunctionNamesWithAnnexKReplacementMatcher = hasAnyName(
+  "::asctime", "::ctime", "::fopen", "::freopen", "::bsearch", "::fprintf",

Is this ordering specific in any way? Is the rule listing them in this order? 
If not, can we have them listed alphabetically, for easier search and potential 
later change?



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-msc24-c.rst:10
+For the listed functions, an alternative, more secure replacement is 
suggested, if available.
+The checker heavily relies on the functions from annex K (Bounds-checking 
interfaces) of C11.
+

(And consistent capitalisation later.)



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-msc24-c.rst:29
+Both macros have to be defined to suggest replacement functions from annex K. 
`__STDC_LIB_EXT1__` is
+defined by the library implementation, and `__STDC_WANT_LIB_EXT1__` must be 
define to "1" by the user 
+before including any system headers.




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

https://reviews.llvm.org/D91000

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

[PATCH] D114197: [clang-tidy] Fix false positives involving type aliases in `misc-unconventional-assign-operator` check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator.cpp:151
+  using Alias3 = TemplateTypeAlias;
+  Alias3 =(int) { return *this; }
+};

This is a no-warn due to the parameter being a completely unrelated type, 
right? Might worth a comment. I don't see at first glance why a warning should 
not happen here.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator.cpp:152
+  Alias3 =(int) { return *this; }
+};

What about `Alias3& operator =(const Alias1&) { return *this; 
}`? That should trigger a warning as it is an unrelated type, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114197

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

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:54-55
const NestedNameSpecifier *Right) {
-  llvm::FoldingSetNodeID LeftID, RightID;
-  Left->Profile(LeftID);
-  Right->Profile(RightID);
-  return LeftID == RightID;
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==

(Wrt. to the comment about not typing stuff out directly, I guess `L` and `R` 
here will be `X` and `X` and thus not the same.)



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:54-57
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==
+ R->getCanonicalTypeUnqualified();

whisperity wrote:
> (Wrt. to the comment about not typing stuff out directly, I guess `L` and `R` 
> here will be `X` and `X` and thus not the same.)
(Style nit to lessen the indent depth, until C++17...)



Comment at: 
clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:132-134
+const DeclRefExpr *DeclRef1 = cast(Left);
+const DeclRefExpr *DeclRef2 = cast(Right);
+return areEquivalentDeclRefs(DeclRef1, DeclRef2);

(Style nit. Or `LeftDR`, `RightDR`.)



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:822
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;

Are there any more ways we could trick this check to cause false positives? 
Something through which it doesn't look the same but still refers to the same 
thing?

At the top of my head I'm thinking about something along the lines of this:

```lang=cpp
template 
struct X {
  template 
  static constexpr bool same = std::is_same_v;
};

X ch() { return X{}; }
X i() { return X{}; }

void test() {
  coin ? ch().same : i().same;
}
```

So the "type name" where we want to look up isn't typed out directly.
Or would these still be caught (and thus fixed) by looking at a 
`NestedNameSpecifier`?


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

https://reviews.llvm.org/D114622

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

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

I haven't looked at the patch in detail yet, but I know I've run into the issue 
this aims to fix when developing a Tidy check!

There is a `// NOLINTNEXTLINE(misc-redundant-expression)` directive in the 
source file 
`clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp` 
right before a `static_assert` that checks some `type_traits`-y stuff. Should 
be at line 510 -- according to my local checkout. Could you please remove that 
line and have that change be part of this patch too? I did a grep on the 
project and that's the only "user-side" mention to the misc-redundant-... 
check. Running Tidy on Tidy itself afterwards would turn out to be a nice 
real-world test that the fix indeed works. 


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

https://reviews.llvm.org/D114622

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

[PATCH] D114602: [clang-tidy] Improve documentation of bugprone-unhandled-exception-at-new [NFC]

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:8-10
 Calls to ``new`` can throw exceptions of type ``std::bad_alloc`` that should
 be handled by the code. Alternatively, the nonthrowing form of ``new`` can be
 used. The check verifies that the exception is handled in the function

`by the code` is either superfluous or misleading. Did you mean that the 
exception should be caught and handled in the same scope where the allocation 
took place?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:16-17
+
+The exception handler is checked for types ``std::bad_alloc``,
+``std::exception``, and catch-all handler.
 The check assumes that any user-defined ``operator new`` is either





Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:19-20
 The check assumes that any user-defined ``operator new`` is either
 ``noexcept`` or may throw an exception of type ``std::bad_alloc`` (or derived
 from it). Other exception types or exceptions occurring in the object's
 constructor are not taken into account.

What does

> exception types or exceptions

mean?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:26
+  int *f() noexcept {
+int *p = new int[1000]; // warning: exception handler is missing
+// ...

Is that the warning message the check prints?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:35
+
+  int *f1() { // no 'noexcept'
+int *p = new int[1000]; // no warning: exception can be handled outside




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114602

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

[PATCH] D113804: [clang-tidy] Fix behavior of `modernize-use-using` with nested structs/unions

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

`hasParent()` is the //direct// upwards traversal, right? I remember some 
discussion about matchers like that being potentially costly. But I can't find 
any description of this, so I guess it's fine, rewriting the matcher to have 
the opposite logic 
(`decl(hasDescendant(...the-real-matcher-here...)).bind("blah")`) would just 
make everything ugly for no tangible benefit.




Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:29
+  Finder->addMatcher(typedefDecl(unless(isInstantiated()),
+ hasParent(decl().bind("parent-decl")))
+ .bind("typedef"),

(Nit: Once we have multiple uses of an identifier, it's better to hoist it to a 
definition and reuse it that way, to prevent future typos introducing arcane 
issues. Generally, a `static constexpr llvm::StringLiteral` in the TU's global 
scope, is sufficient for that.)



Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:50-52
+  if (!ParentDecl) {
+return;
+  }

(Style: Elid braces.)



Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:132-137
 bool Invalid;
-Type = std::string(
-Lexer::getSourceText(CharSourceRange::getTokenRange(LastTagDeclRange),
- *Result.SourceManager, getLangOpts(), ));
+Type = std::string(Lexer::getSourceText(
+CharSourceRange::getTokenRange(LastTagDeclRange->second),
+*Result.SourceManager, getLangOpts(), ));
 if (Invalid)
   return;

(Nit: if the source ranges are invalid, a default constructed `StringRef` is 
returned, which is empty and converts to a default-constructed `std::string`.)


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

https://reviews.llvm.org/D113804

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

[PATCH] D113518: [clang][Sema] Create delegating constructors even in templates

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D113518#3155040 , @carlosgalvezp 
wrote:

> Thanks for the finding and sorry for delaying the review, I didn't know that 
> :( Do you know if there's an option for signaling "LGTM but I want someone 
> else to review?". Otherwise I can just leave a comment and don't accept the 
> revision.

There is an action "resign from review", but as I have taken you off explicitly 
and by force, no need for that.  I just wrote the comment FYI so that you know 
I'm not just interfering destructively! (And due to no accepting reviewers 
remaining acting as such, the patch automatically went back to //"now require 
review to proceed"//.)


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

https://reviews.llvm.org/D113518

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

[PATCH] D113518: [clang][Sema] Create delegating constructors even in templates

[Whisperity via Phabricator via cfe-commits]

whisperity edited reviewers, added: Quuxplusone; removed: carlosgalvezp, 
whisperity.
whisperity added subscribers: carlosgalvezp, whisperity.
whisperity added a comment.
This revision now requires review to proceed.

(@carlosgalvezp Removing you from reviewers so the patch shows up as not yet 
reviewed for others! The significant part is the Sema which should be reviewed.)


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

https://reviews.llvm.org/D113518

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

[PATCH] D114292: [clang-tidy] Fix `altera-struct-pack-align` check for empty structs

[Whisperity via Phabricator via cfe-commits]

whisperity requested changes to this revision.
whisperity added a comment.
This revision now requires changes to proceed.

I believe this is worth a note in the `ReleaseNotes.rst` file. People who might 
have disabled the check specifically for the reason outlined in the PR would 
get to know it's safe to reenable it.




Comment at: clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp:77
+  CharUnits::fromQuantity(std::max(
+  ceil((float)TotalBitSize / CharSize), 1));
   CharUnits MaxAlign = CharUnits::fromQuantity(

If we are changing this, can we make this more C++-y?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114292

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

This generally looks fine for me, but I'd rather have other people who are more 
knowledgeable about the standard's intricacies looked at it too.

AFAIK Aaron is busy this week and the next (?) due to C++ Committee meetings, 
or something like that. And I bet after such meetings everyone would take a few 
more days off from looking at C++ stuff.


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

https://reviews.llvm.org/D107450

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Thank you, this is getting much better!




Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:105-108
+  const auto *PositiveCheck = Result.Nodes.getNodeAs("positive");
+  const auto *NegativeCheck = Result.Nodes.getNodeAs("negative");
+  bool Negated = NegativeCheck != nullptr;
+  const auto *Check = Negated ? NegativeCheck : PositiveCheck;

avogelsgesang wrote:
> whisperity wrote:
> > `Comparison` instead of `Check`? These should be matching the 
> > `binaryOperator`, right?
> In most cases, they match the `binaryOperator`. For the first pattern they 
> match the `implicitCastExpr`, though
> 
> Given this is not always a `binaryOperator`, should I still rename it to 
> `Comparison`?
`Comparison` is okay. Just `Check`, `Positive` and `Negative` seem a bit 
ambiguous at first glance.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst:6
+
+Finds usages of `container.count()` and `container.find() == container.end()` 
which should be replaced by a call to the `container.contains()` method 
introduced in C++ 20.
+

avogelsgesang wrote:
> whisperity wrote:
> > whisperity wrote:
> > > Same comment about the backtick count and how you would like the 
> > > rendering to be. Please build the documentation locally and verify 
> > > visually, as both ways most likely compile without warnings or errors.
> > 
> > Please build the documentation locally [...]
> 
> How do I actually do that?
You need to install [[ http://pypi.org/project/Sphinx | `Sphinx`]]. The easiest 
is to install it with //`pip`//, the Python package manager. Then, you tell 
LLVM CMake to create the build targets for the docs, and run it:

```lang=bash
~$ python3 -m venv sphinx-venv
~$ source ~/sphinx-venv/bin/activate
(sphinx-venv) ~$ pip install sphinx
(sphinx-venv) ~$ cd LLVM_BUILD_DIR
(sphinx-venv) LLVM_BUILD_DIR/$ cmake . -DLLVM_BUILD_DOCS=ON 
-DLLVM_ENABLE_SPHINX=ON
# configuration re-run
(sphinx-venv) LLVM_BUILD_DIR/$ cmake --build . -- docs-clang-tools-html
(sphinx-venv) LLVM_BUILD_DIR/$ cd ~
(sphinx-venv) ~$ deactivate
~$ # you are now back in HOME without the virtual env being active
```

The docs will be available starting from 
`${LLVM_BUILD_DIR}/tools/clang/tools/extra/docs/html/clang-tidy/index.html`.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:115
+
+// This is effectively a membership check, as the result is implicitely casted 
to `bool`
+bool returnContains(std::map ) {

Make sure to format it against 80-line, and also, we conventionally write full 
sentences in comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:129
+
+// Check that we are not confused by aliases
+namespace s2 = std;





Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:174
+
+// The following map has the same interface like `std::map`
+template 





Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:189
+// NO-WARNING.
+// CHECK-FIXES: if (MyMap.count(0))
+return nullptr;

If a fix is not generated, why is this line here?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:33-35
+
+using namespace std;
+

avogelsgesang wrote:
> whisperity wrote:
> > Tests are to guard our future selves from breaking the system, so perhaps 
> > two tests that involve having `std::` typed out, and also using a different 
> > container that's not `std::whatever` would be useful.
> > 
> > 
> > 
> > Do you think it would be worthwhile to add matching any user-defined object 
> > which has a `count()` and a `contains()` method (with the appropriate 
> > number of parameters) later? So match not only the few standard containers, 
> > but more stuff?
> > 
> > It doesn't have to be done now, but having a test for `MyContainer` not in 
> > `std::` being marked `// NO-WARNING.` or something could indicate that we 
> > purposefully don't want to go down that road just yet.
> > so perhaps two tests that involve having std:: typed out
> 
> rewrote the tests, such that most of them use fully-qualified types. Also 
> added a few test cases involving type defs and namespace aliases (this 
> actually uncovered a mistake in the matcher)
> 
> > Do you think it would be worthwhile to add matching any user-defined object 
> > which has a count() and a contains() method (with the appropriate number of 
> > parameters) later?
> 
> Not sure. At least not for the code base I wrote this check for...
> 
> > having a test for MyContainer not in std:: being marked // NO-WARNING. or 
> > something could indicate that we purposefully don't want to go 

[PATCH] D113558: [clang-tidy] Fix cppcoreguidelines-virtual-base-class-destructor in macros

[Whisperity via Phabricator via cfe-commits]

whisperity accepted this revision.
whisperity added a comment.

I'm not sure if it is a good idea to merge things on a Friday, but I am 
comfortable with this, let's get the check crash less.


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

https://reviews.llvm.org/D113558

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

[PATCH] D114254: [libtooling][clang-tidy] Fix crashing on rendering invalid SourceRanges

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp:102
+  EXPECT_EQ("invalid->invalid", Errors[0].Message.Message);
+  EXPECT_EQ(0ul, Errors[0].Message.Ranges.size());
+

`EXPECT_TRUE(Errors[0].Message.Ranges.empty());`? But I'm not sure if the data 
structure used there supports this.


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

https://reviews.llvm.org/D114254

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

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp:42-46
+/// Remove all '_' characters at the beginning of the identifier. Only reserved
+/// identifiers are allowed to start with these.
+static StringRef dropLeadingUnderscores(StringRef Identifier) {
+  return Identifier.drop_while([](char c) { return c == '_'; });
+}

Is this true? At least in C++ (and perhaps in C) I believe `_foo` is a 
non-reserved identifier, only `__foo` or `_Foo` would be reserved.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderGuard.h:41
 
+  /// Ensure that the provided header guard is a valid identifier.
+  static std::string sanitizeHeaderGuard(StringRef Guard);

valid **non-reserved**


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114149

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

[PATCH] D113251: [analyzer][doc] Add user documenation for taint analysis

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang/docs/analyzer/checkers.rst:2341-2342
+
+Default propagations defined by `GenericTaintChecker`:
+``atoi``, ``atol``, ``atoll``, ``fgetc``, ``fgetln``, ``fgets``, ``fscanf``, 
``sscanf``, ``getc``, ``getc_unlocked``, ``getdelim``, ``getline``, ``getw``, 
``pread``, ``read``, ``strchr``, ``strrchr``, ``tolower``, ``toupper``
+

What does it mean that these are "default propagations"? That taint passes 
through calls to them trivially?



Comment at: clang/docs/analyzer/checkers.rst:2345
+Default sinks defined in `GenericTaintChecker`:
+``printf``, ``setproctitle``, ``system``, ``popen``, ``execl``, ``execle``, 
``execlp``, ``execv``, ``execvp``, ``execvP``, ``execve``, ``dlopen``, 
``memcpy``, ``memmove``, ``strncpy``, ``strndup``, ``malloc``, ``calloc``, 
``alloca``, ``memccpy``, ``realloc``, ``bcopy``
+

`execvp` and `execvP`? What is the capital `P` for? I can't find this overload 
in the POSIX docs.



Comment at: clang/docs/analyzer/checkers.rst:2353
+
+External taint configuration is in `YAML `_ format. The 
taint-related options defined in the config file extend but do not override the 
built-in sources, rules, sinks.
+

Perhaps we should link to the in-house YAML doc 
(http://llvm.org/docs/YamlIO.html#introduction-to-yaml) first, and have the 
user move to other sources from there?



Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:5
+
+Clang Static Analyzer uses taint analysis to detect security-related issues in 
code.
+The backbone of taint analysis in Clang is the `GenericTaintChecker`, which 
the user can access via the :ref:`alpha-security-taint-TaintPropagation` 
checker alias and this checker has a default taint-related configuration.

//The// Clang Static [...]?



Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:5
+
+Clang Static Analyzer uses taint analysis to detect security-related issues in 
code.
+The backbone of taint analysis in Clang is the `GenericTaintChecker`, which 
the user can access via the :ref:`alpha-security-taint-TaintPropagation` 
checker alias and this checker has a default taint-related configuration.

whisperity wrote:
> //The// Clang Static [...]?
uses, or can use?



Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:6
+Clang Static Analyzer uses taint analysis to detect security-related issues in 
code.
+The backbone of taint analysis in Clang is the `GenericTaintChecker`, which 
the user can access via the :ref:`alpha-security-taint-TaintPropagation` 
checker alias and this checker has a default taint-related configuration.
+The checker also provides a configuration interface for extending the default 
settings by providing a configuration file in `YAML `_ 
format.

Clang -> Clang SA / CSA?



Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:6
+Clang Static Analyzer uses taint analysis to detect security-related issues in 
code.
+The backbone of taint analysis in Clang is the `GenericTaintChecker`, which 
the user can access via the :ref:`alpha-security-taint-TaintPropagation` 
checker alias and this checker has a default taint-related configuration.
+The checker also provides a configuration interface for extending the default 
settings by providing a configuration file in `YAML `_ 
format.

whisperity wrote:
> Clang -> Clang SA / CSA?
So the checker has the defaults all the time, or only by enabling the alias can 
I get the defaults?



Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:7
+The backbone of taint analysis in Clang is the `GenericTaintChecker`, which 
the user can access via the :ref:`alpha-security-taint-TaintPropagation` 
checker alias and this checker has a default taint-related configuration.
+The checker also provides a configuration interface for extending the default 
settings by providing a configuration file in `YAML `_ 
format.
+This documentation describes the syntax of the configuration file and gives 
the informal semantics of the configuration options.

Ditto about YAML.



Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:13
+
+.. _taint-configuration-overview
+

I think this wanted to be an anchor? Also see how the syntax highlighting broke 
in the next section.



Comment at: clang/docs/analyzer/user-docs/TaintAnalysisConfiguration.rst:23
+
+.. _taint-configuration-example:
+

AFAIK, RST anchors are global identifiers for the project, so maybe we should 
take care in adding "clang-sa" or some other sort of namespaceing...

(Although this might depend