[PATCH] D53334: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too

[Whisperity via Phabricator via cfe-commits]

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

Yes, down the line I realised the function is not needed. (It emits a 
diagnostic because the diagnostic comes from comparing the AST file's config 
blocks to the current (at the time of import) compilation.)

I have simplified the tests.
If @rsmith has no objections (or sadly time to look at this), please commit on 
my behalf, I have no SVN access.


https://reviews.llvm.org/D53334

Files:
  lib/Frontend/CompilerInstance.cpp
  test/Modules/mismatch-diagnostics.cpp


Index: test/Modules/mismatch-diagnostics.cpp
===
--- /dev/null
+++ test/Modules/mismatch-diagnostics.cpp
@@ -0,0 +1,22 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/prebuilt_modules
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple  \
+// RUN: -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -emit-module-interface -pthread -DBUILD_MODULE  \
+// RUN: %s -o %t/prebuilt_modules/mismatching_module.pcm
+//
+// RUN: not %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DCHECK_MISMATCH \
+// RUN: %s 2>&1 | FileCheck %s
+
+#ifdef BUILD_MODULE
+export module mismatching_module;
+#endif
+
+#ifdef CHECK_MISMATCH
+import mismatching_module;
+// CHECK: error: POSIX thread support was enabled in PCH file but is currently 
disabled
+// CHECK-NEXT: module file {{.*}}/mismatching_module.pcm cannot be loaded due 
to a configuration mismatch with the current compilation
+#endif
+
Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1736,7 +1736,9 @@
 // module cache, we don't know how to rebuild modules.
 unsigned ARRFlags = Source == ModuleCache ?
 ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing :
-ASTReader::ARR_ConfigurationMismatch;
+Source == PrebuiltModulePath ?
+0 :
+ASTReader::ARR_ConfigurationMismatch;
 switch (ModuleManager->ReadAST(ModuleFileName,
Source == PrebuiltModulePath
? serialization::MK_PrebuiltModule


Index: test/Modules/mismatch-diagnostics.cpp
===
--- /dev/null
+++ test/Modules/mismatch-diagnostics.cpp
@@ -0,0 +1,22 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/prebuilt_modules
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple  \
+// RUN: -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -emit-module-interface -pthread -DBUILD_MODULE  \
+// RUN: %s -o %t/prebuilt_modules/mismatching_module.pcm
+//
+// RUN: not %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DCHECK_MISMATCH \
+// RUN: %s 2>&1 | FileCheck %s
+
+#ifdef BUILD_MODULE
+export module mismatching_module;
+#endif
+
+#ifdef CHECK_MISMATCH
+import mismatching_module;
+// CHECK: error: POSIX thread support was enabled in PCH file but is currently disabled
+// CHECK-NEXT: module file {{.*}}/mismatching_module.pcm cannot be loaded due to a configuration mismatch with the current compilation
+#endif
+
Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1736,7 +1736,9 @@
 // module cache, we don't know how to rebuild modules.
 unsigned ARRFlags = Source == ModuleCache ?
 ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing :
-ASTReader::ARR_ConfigurationMismatch;
+Source == PrebuiltModulePath ?
+0 :
+ASTReader::ARR_ConfigurationMismatch;
 switch (ModuleManager->ReadAST(ModuleFileName,
Source == PrebuiltModulePath
? serialization::MK_PrebuiltModule
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In https://reviews.llvm.org/D53974#1294385, @ztamas wrote:

> I also tested on LLVm code.
>  The output is here:
>  https://gist.github.com/tzolnai/fe4f23031d3f9fdbdbf1ee38abda00a4
>
> I found 362 warnings.
>
> Around 95% of these warnings are similar to the next example:
>
>   /home/zolnai/lohome/llvm/lib/Support/Chrono.cpp:64:24: warning: loop 
> variable has narrower type 'unsigned int' than iteration's upper bound 
> 'size_t' (aka 'unsigned long') [bugprone-too-small-loop-variable]
> for (unsigned I = 0; I < Style.size(); ++I) {
>
>
> Where the loop variable has an `unsigned int` type while in the loop 
> condition it is compared with a container size which has `size_t` type. The 
> actual size method can be `std::string::length()` or `array_lengthof()` too.
>
> //[snip snip]//
>
> I can't see similar false positives what LibreOffice code produces.


I am fairly concerned the example with unsigned use for container iteration are 
not false positives, just examples of bad happenstance code which never breaks 
under real life applications due to uint32_t being good enough but is actually 
not type-safe.
Those examples that I kept in my quote are especially bad and should be fixed 
eventually...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974



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

[PATCH] D53069: [analyzer][www] Update avaible_checks.html

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Let's register the ID...

Superseded by https://reviews.llvm.org/D54429.


https://reviews.llvm.org/D53069



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

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like elements

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Yeah, it seems upstream has moved away due to @Szelethus' implementation of a 
much more manageable "checker dependency" system. You most likely will have to 
rebase your patch first, then check what you missed which got added to other 
merged, existing checkers.


Repository:
  rC Clang

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

https://reviews.llvm.org/D50488



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

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like keys

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:88
+
+  if (!II->getName().equals("sort"))
+return;

Brrr... `equals`. StringRef has a `==` and `!=` operator which accepts string 
literals on the other side, which would result in a more concise code.

Also, this heuristic can be applied without extra changes (apart from those 
mentioned by NoQ and might be mentioned later by others) to other sorting 
functions, such as `std::stable_sort` and `std::stable_partition`. Perhaps it 
would be worthy to enable checking those functions too.


Repository:
  rC Clang

https://reviews.llvm.org/D50488



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

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like keys

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

The basics of the heuristics look okay as comparing pointers from 
non-continuous block of memory is undefined, it would be worthy to check if no 
compiler warning (perhaps by specifying `-W -Wall -Wextra -Weverything` and 
various others of these enable-all flags!) is emitted if `std::sort` is 
instantiated for such a use case.

There is a chance for a serious false positive with the current approach! One 
thing which you should add as a **FIXME**, and implement in a follow-up patch. 
Using `std::sort`, `std::unique` and then `erase` is common technique for 
deduplicating a container (which in some cases might even be quicker than 
using, let's say `std::set` ). In 
case my container contains arbitrary memory addresses (e.g. multiple things 
give me different stuff but might give the same thing multiple times) which I 
don't want to do things with more than once, I might use 
`sort`-`unique`-`erase` and the `sort` call will emit a report.


Repository:
  rC Clang

https://reviews.llvm.org/D50488



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

[PATCH] D52742: [analyzer][PlistMacroExpansion] Part 1.: New expand-macros flag

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist:6
+ clang_version
+clang version 8.0.0 (http://mainstream.inf.elte.hu/Szelethus/clang 
85a6dda64587a5a18482f091cbcf020fbd3ec1dd) (https://github.com/llvm-mirror/llvm 
1ffbf26a1a0a190d69327af875a3337b74a2ce82)
+ diagnostics

You are diffing against a hardcoded output file which contains //a git hash//! 
Won't this break at the next commit?!


Repository:
  rC Clang

https://reviews.llvm.org/D52742



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

[PATCH] D52742: [analyzer][PlistMacroExpansion] Part 1.: New expand-macros flag

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist:346
+ 
+  
/home/eumakri/Documents/2codechecker_dev_env/llvm/tools/clang/test/Analysis/plist-macros-with-expansion.cpp
+ 

Same here, as @xazax.hun mentioned. (Damn when I make a checker I'll be careful 
to ensure my paths don't contain swearwords... 😜 )

Maybe we should carefully analyse what is exactly needed from the plist and 
throw the rest, just to make the whole file smaller. Plists are pretty bloaty 
already...


https://reviews.llvm.org/D52742



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

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Whisperity via Phabricator via cfe-commits]

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

Have been looked at this far and wide too many times now. I say we roll this 
out and fix and improve later on, lest this only going into Clang 72. Results 
look promising, let's hope users start reporting good findings too.

Considering Tidy has no `alpha` group the name `bugprone` seems like a 
double-edged sword, as what is bugprone, the checker or what it finds? 😜


https://reviews.llvm.org/D45050



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

[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.
Herald added subscribers: donat.nagy, Szelethus, mikhail.ramalho, rnkovacs, 
szepet.
Herald added a reviewer: george.karpenkov.

//Soft ping.//


https://reviews.llvm.org/D33672



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

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@aaron.ballman Neither I nor @Charusso has commit rights, could you please 
commit this in our stead?


https://reviews.llvm.org/D45050



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

[PATCH] D53334: [Frontend] Show diagnostics on prebuilt module configuration mismatch too

[Whisperity via Phabricator via cfe-commits]

whisperity created this revision.
whisperity added reviewers: rsmith, doug.gregor.
whisperity added a project: clang.
Herald added subscribers: Szelethus, dkrupp, rnkovacs.

The current version only emits  the below error for a module (attempted to be 
loaded) from the `prebuilt-module-path`:

  error: module file blabla.pcm cannot be loaded due to a configuration 
mismatch with the current compilation [-Wmodule-file-config-mismatch]

With this change, if the prebuilt module is used, we allow the proper 
diagnostic behind the configuration mismatch to be shown.

  error: POSIX thread support was disabled in PCH file but is currently enabled
  error: module file blabla.pcm cannot be loaded due to a configuration 
mismatch with the current compilation [-Wmodule-file-config-mismatch]

(A few lines later an error is emitted anyways, so there is no reason not to 
complain for configuration mismatches if a config mismatch is found and kills 
the build.)


Repository:
  rC Clang

https://reviews.llvm.org/D53334

Files:
  lib/Frontend/CompilerInstance.cpp


Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1724,7 +1724,9 @@
 // module cache, we don't know how to rebuild modules.
 unsigned ARRFlags = Source == ModuleCache ?
 ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing :
-ASTReader::ARR_ConfigurationMismatch;
+Source == PrebuiltModulePath ?
+0 :
+ASTReader::ARR_ConfigurationMismatch;
 switch (ModuleManager->ReadAST(ModuleFileName,
Source == PrebuiltModulePath
? serialization::MK_PrebuiltModule


Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1724,7 +1724,9 @@
 // module cache, we don't know how to rebuild modules.
 unsigned ARRFlags = Source == ModuleCache ?
 ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing :
-ASTReader::ARR_ConfigurationMismatch;
+Source == PrebuiltModulePath ?
+0 :
+ASTReader::ARR_ConfigurationMismatch;
 switch (ModuleManager->ReadAST(ModuleFileName,
Source == PrebuiltModulePath
? serialization::MK_PrebuiltModule
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52790: [analyzer][PlistMacroExpansion] New flag to convert macro expansions to events

[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 subscriber: donat.nagy.

Remove the custom file paths and URLs but other than that this is pretty 
trivial.


https://reviews.llvm.org/D52790



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

[PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

With regards to https://reviews.llvm.org/D53352: you can change the diff 
related to a patch whilst keeping discuccion and metadata of the diff.

Please add a generic description to the diff for an easy skimming on what you 
are accomplishing.

If I get this right, your tool will spit out a CPP file that is only include 
directives and perhaps the related conditional logic, or the final output of 
your tool is a file list? This is different than the `-M` flags in a way that 
it keeps conditions sane, rather than spitting out what includes were used if 
the input, with regards to the compiler options, was to be compiled?

Have you checked the tool //Include What You Use//? I'm curious in what way the 
mishappenings of that tool present themselves in yours. There were some 
challenges not overcome in a good way in IWYU, their readme goes into a bit 
more detail on this.


Repository:
  rC Clang

https://reviews.llvm.org/D53354



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

[PATCH] D52988: [analyzer][PlistMacroExpansion] Part 5.: Support for # and ##

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Looks good.

What happens if the macro is to stringify a partially string argument?

  #define BOOYAH(x) #x ";
  
  ... 
  
  std::string foo = BOOYAH(blabla)
  std::string foo2 = BOOYAH("blabla)
  int x = 2;

Not sure if these cases are even valid C(XX), but if they are, we should test.

An idea for a follow-up patch if it's not that hard work: you mentioned your 
original approach with that madness in the HTML printer. Perhaps it could be 
refactored to use this implementation too and thus we'll only have 9 places 
where macro expansion logic is to be maintained, not 10. 😈


https://reviews.llvm.org/D52988



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:238
+  /// message on that constraint being changed.
+  bool isChangedOrInsertConstraint(ConstraintMap &Constraints, const Stmt 
*Cond,
+   StringRef Message) const {

`insertOrUpdateContraintMessage`

and mention in the documentation that it returns whether or not the actual 
insertion or update change took place


https://reviews.llvm.org/D53076



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

[PATCH] D53334: [Frontend] Show diagnostics on prebuilt module configuration mismatch too

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@dblaikie I have created a test, but unfortunately `%clang_cpp` in LIT invokes 
`clang --driver-mode=cpp` which is not the same as if `clang++` is called. I'm 
trying to construct the following command-line

`clang++ -fmodules-ts -fprebuilt-module-path=%t/mods --precompile -c file.cppm 
-o file.pcm`

However, using `%clang_cc1` I can't get it to accept `--precompile` as a valid 
argument, and using `%clang_cpp` I get an "unused argument" warning for 
`--precompile` and the output file is just a preprocessed output (like `-E`), 
which will, of course, cause errors, but not the errors I am wanting to test 
about.


Repository:
  rC Clang

https://reviews.llvm.org/D53334



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

[PATCH] D53334: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 170832.
whisperity retitled this revision from "[Frontend] Show diagnostics on prebuilt 
module configuration mismatch too" to "[Frontend/Modules] Show diagnostics on 
prebuilt module configuration mismatch too".
whisperity added a comment.
Herald added a subscriber: jfb.

Updating the diff just in case so that I don't lose the test code.


Repository:
  rC Clang

https://reviews.llvm.org/D53334

Files:
  lib/Frontend/CompilerInstance.cpp
  test/Modules/Inputs/module-mismatch.cppm
  test/Modules/mismatch-diagnostics.cpp


Index: test/Modules/mismatch-diagnostics.cpp
===
--- /dev/null
+++ test/Modules/mismatch-diagnostics.cpp
@@ -0,0 +1,29 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/prebuilt_modules
+//
+// RUN: exit 0
+//
+// RUN: %clang_cpp -triple %itanium_abi_triple \
+// RUN: -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -pthread -DHAS_PTHREAD=1 \
+// RUN: --precompile -c %S/Inputs/module-mismatch.cppm \
+// RUN: -o %t/prebuilt_modules/module_mismatch.pcm
+//
+// RUN: %clang_cpp -triple %itanium_abi_triple \
+// RUN: -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: --precompile -c %S/Inputs/module-mismatch.cppm \
+// RUN: -o %t/prebuilt_modules/module_no_mismatch.pcm
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DMISMATCH_CHCEK=1 %s
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules %s
+
+#ifdef MISMATCH_CHECK
+import module_mismatch;
+// expected-error {{foo}}
+#else
+import module_no_mismatch; // no-warning
+#endif
+
Index: test/Modules/Inputs/module-mismatch.cppm
===
--- /dev/null
+++ test/Modules/Inputs/module-mismatch.cppm
@@ -0,0 +1,13 @@
+#ifdef HAS_PTHREAD
+export module module_mismatch;
+#else
+export module module_no_mismatch;
+#endif
+
+export bool hasPthreads() {
+#ifdef HAS_PTHREAD
+return true;
+#else
+return false;
+#endif
+}
Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1736,7 +1736,9 @@
 // module cache, we don't know how to rebuild modules.
 unsigned ARRFlags = Source == ModuleCache ?
 ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing :
-ASTReader::ARR_ConfigurationMismatch;
+Source == PrebuiltModulePath ?
+0 :
+ASTReader::ARR_ConfigurationMismatch;
 switch (ModuleManager->ReadAST(ModuleFileName,
Source == PrebuiltModulePath
? serialization::MK_PrebuiltModule


Index: test/Modules/mismatch-diagnostics.cpp
===
--- /dev/null
+++ test/Modules/mismatch-diagnostics.cpp
@@ -0,0 +1,29 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/prebuilt_modules
+//
+// RUN: exit 0
+//
+// RUN: %clang_cpp -triple %itanium_abi_triple \
+// RUN: -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -pthread -DHAS_PTHREAD=1 \
+// RUN: --precompile -c %S/Inputs/module-mismatch.cppm \
+// RUN: -o %t/prebuilt_modules/module_mismatch.pcm
+//
+// RUN: %clang_cpp -triple %itanium_abi_triple \
+// RUN: -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: --precompile -c %S/Inputs/module-mismatch.cppm \
+// RUN: -o %t/prebuilt_modules/module_no_mismatch.pcm
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DMISMATCH_CHCEK=1 %s
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules %s
+
+#ifdef MISMATCH_CHECK
+import module_mismatch;
+// expected-error {{foo}}
+#else
+import module_no_mismatch; // no-warning
+#endif
+
Index: test/Modules/Inputs/module-mismatch.cppm
===
--- /dev/null
+++ test/Modules/Inputs/module-mismatch.cppm
@@ -0,0 +1,13 @@
+#ifdef HAS_PTHREAD
+export module module_mismatch;
+#else
+export module module_no_mismatch;
+#endif
+
+export bool hasPthreads() {
+#ifdef HAS_PTHREAD
+return true;
+#else
+return false;
+#endif
+}
Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1736,7 +1736,9 @@
 // module cache, we don't know how to rebuild modules.
 unsigned ARRFlags = Source == ModuleCache ?
 ASTReader::ARR_OutOfDate | ASTReader::ARR_Missing :
-ASTReade

[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] 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] 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] 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] 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] 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] 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 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] 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 representation

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp:26
 
+  ast_matchers::internal::Matcher InheritsVirtualDestructor =
+  hasAnyBase(hasType(cxxRecordDecl(has(cxxDestructorDecl(isVirtual());

carlosgalvezp wrote:
> aaron.ballman wrote:
> > I'm confused as to why this is necessary -- this AST matcher calls through 
> > to `CXXMethodDecl::isVirtual()` which is different from 
> > `isVirtualAsWritten()` and should already account for inheriting the 
> > virtual specifier.
> In the Bug report, @whisperity mentioned this problem could be somewhere else:
> 
> > Nevertheless, if you check the AST for the test code at 
> > http://godbolt.org/z/6MqG4Kb85, we can see that the derived destructor is 
> > in fact **not** marked virtual. So it's Sema which does not give this flag 
> > to the AST-clients properly.
> 
> I don't even know what Sema is so I don't really know how to fix it at that 
> level, and I wonder if it would break other things.
> 
> If anyone has some time to walk me through where the fix should go I'm happy 
> to try it out!
`Sema` is the Clang module responsible for **sema**ntic analysis, hence the 
name. It is an egregiously complex and huge class (you know something's weird 
when a header of 80k lines is implemented over like 20 separate CPP files...) 
that is basically responsible for building the AST.

It was just a hunch from my side because I went into Godbolt to try seeing why 
the matching logic would fail!



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp:26
 
+  ast_matchers::internal::Matcher InheritsVirtualDestructor =
+  hasAnyBase(hasType(cxxRecordDecl(has(cxxDestructorDecl(isVirtual());

whisperity wrote:
> carlosgalvezp wrote:
> > aaron.ballman wrote:
> > > I'm confused as to why this is necessary -- this AST matcher calls 
> > > through to `CXXMethodDecl::isVirtual()` which is different from 
> > > `isVirtualAsWritten()` and should already account for inheriting the 
> > > virtual specifier.
> > In the Bug report, @whisperity mentioned this problem could be somewhere 
> > else:
> > 
> > > Nevertheless, if you check the AST for the test code at 
> > > http://godbolt.org/z/6MqG4Kb85, we can see that the derived destructor is 
> > > in fact **not** marked virtual. So it's Sema which does not give this 
> > > flag to the AST-clients properly.
> > 
> > I don't even know what Sema is so I don't really know how to fix it at that 
> > level, and I wonder if it would break other things.
> > 
> > If anyone has some time to walk me through where the fix should go I'm 
> > happy to try it out!
> `Sema` is the Clang module responsible for **sema**ntic analysis, hence the 
> name. It is an egregiously complex and huge class (you know something's weird 
> when a header of 80k lines is implemented over like 20 separate CPP files...) 
> that is basically responsible for building the AST.
> 
> It was just a hunch from my side because I went into Godbolt to try seeing 
> why the matching logic would fail!
Sadly I'm a bit short on time nowadays due to university and bureaucracy, but 
remember that either Clang-Query is your friend (available on Godbolt, with 
visual highlight of the matched things back in the source code!) and you can 
sandbox match expressions there (`match ...` for matching, `let X ...` for 
saving `...` into `X` to reuse later), or you could do `AnyASTNode->dump()` in 
the code while development to observe what the node is on the output.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp:36
   has(cxxDestructorDecl(isProtected(), unless(isVirtual()))
   .bind("ProblematicClassOrStruct"),
   this);

Nit: `ProblematicRecord` might be a better, and certainly shorter, name, while 
meaning the same thing.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:206
+
+// The following two checks cover bug 
https://bugs.llvm.org/show_bug.cgi?id=51912
+// Forward declarations

aaron.ballman wrote:
> Rather than use a comment, we typically put the test cases into a namespace. 
> e.g.,
> ```
> namespace PR51912 {
> // tests cases for that bug live here
> } // namespace PR51912
> ```
Nit: "PR" could be mistaken for //Pull Request// especially since the project 
now lives on GitHub (even though we don't use pull requests (yet?)), so maybe 
explicitly saying `Bugzilla_51912` or something like that instead would be more 
obvious. And it's worthy to put the link in, still.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:208
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 
'ForwardDeclaredStruct' is publi

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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:254
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' 
of the trivially-copyable type 'int' has no effect; consider changing showInt's 
parameter from 'int &&' to 'const int &'
+}

From `int &`? Isn't the parameter `int &&` there?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:267-270
+void showTmp(Tmp &&) {}
+void testTmp() {
+  Tmp t;
+  showTmp(std::move(t));

Sockke wrote:
> whisperity wrote:
> > Is there a test case covering if there are separate invocations of the 
> > function? Just to make sure that the //consider changing the parameter// 
> > suggestion won't become an annoyance either, and result in conflicts with 
> > other parts of the code.
> The warning will only be given when calling this function and passing in the 
> parameters wrapped with std::move. Is my understanding correct？
> 
Yes, that is what I mean. But I would like to see a test case where the "target 
function" is called **multiple times** with different arguments. One call with 
`std::move(x)`, one with a direct temporary, etc. So we can make sure that if a 
function is called more than 1 time, we can still be sure there won't be bad 
warnings or bad fixits that contradict each other.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:261
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' 
of the trivially-copyable type 'int' has no effect; consider changing showInt's 
parameter from 'int'&& to 'int'&
+  return std::move(a);

Sockke wrote:
> whisperity wrote:
> > MTC wrote:
> > > Change **'int'&&**  -> **'int&&'** and **'int&'** -> **int**. 
> > > 
> > > Make `consider changing showInt's parameter from 'int'&& to 'int'&` as a 
> > > note instead of a warning. And I don't have a strong preference for the 
> > > position of the note, but I personally want to put it in the source 
> > > location of the function definition. and 
> > >>! In D107450#2936824, @MTC wrote:
> > > but I personally want to put it in the source location of the function 
> > > definition
> > 
> > (I think you submitted the comment too early, @MTC, as there's an 
> > unterminated sentence there.)
> > 
> > But I agree with this. It should be put there when we're talking about the 
> > function's signature. The reason being the developer reading the warning 
> > could dismiss a potential false positive earlier if they can also 
> > immediately read (at least a part of...) the function's signature.
> > 
> > @Sockke you can do this by doing **another** `diag()` call with the `Note` 
> > value given as the 3rd parameter. And you can specify the SourceLocation of 
> > the affected parameter for this note.
> > >>! In D107450#2936824, @MTC wrote:
> > > but I personally want to put it in the source location of the function 
> > > definition
> > 
> > (I think you submitted the comment too early, @MTC, as there's an 
> > unterminated sentence there.)
> > 
> > But I agree with this. It should be put there when we're talking about the 
> > function's signature. The reason being the developer reading the warning 
> > could dismiss a potential false positive earlier if they can also 
> > immediately read (at least a part of...) the function's signature.
> > 
> > @Sockke you can do this by doing **another** `diag()` call with the `Note` 
> > value given as the 3rd parameter. And you can specify the SourceLocation of 
> > the affected parameter for this note.
> 
> Yes, it is reasonable to add a note, but would you mind adding this 
> modification to the patch that fixes AutoFix? If you don't mind, I will 
> update it.
I think it is okay if the additional note for this is added in this patch, 
there is no need for a separate patch on that. It helps understand WHY the 
FixIt is wrong or not wrong, anyway.


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] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D110614#3032818 , @carlosgalvezp 
wrote:

> Moved the condition of public-virtual / protected-non-virtual to the check 
> stage instead of the match stage. This way is more similar to the equivalent 
> Sema check and it passes all the tests. Let me know what you think!

I'll let @aaron.ballman take a look at how well the test coverage increased, 
this is a drive-by comment from my part due to being a bit too busy. I'll look 
at this at a later point, hopefully still this week!

Either way, you could create your own local matcher predicate function and use 
it as a submatcher, so you can do the test during matching instead of checking.

Adding

  AST_MATCHER(Decl, myFancyMatcher) {
return Node.something();
  }

to the code will give you `myFancyMatcher()`, which you can embed. First 
parameter is the node type you want to match on. Inside the `{ }` you are 
essentially within a function's scope, so you can write arbitrary code, 
returning true is you want to match.

If you check the AST Matcher library header, it uses this same macro 
extensively. Perhaps you could check how the `isVirtual` matcher is implemented 
there?


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

https://reviews.llvm.org/D110614

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

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D110614#3038519 , @carlosgalvezp 
wrote:

> Hi! Are there any more issues I should address? It would be nice to get it 
> merged since it fixes quite a few FPs.

I'm not seeing anything immediate to add. I like the proper printing with the 
template parameters. 🙂
Just in case: add another set of tests where the template instantiation (both 
diagnosed and not diagnosed) is behind a `typedef`, just to see how well the 
diagnostic is printed and generated. Otherwise, I'm happy with this.

> `// Fixes https://bugs.llvm.org/show_bug.cgi?id=51912`

I still think that maybe this whole fiasco is worth either a bug report against 
`Sema` on its own, or just a question-phrased mail to `cfe-dev`.


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

https://reviews.llvm.org/D110614

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

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D108893#3111674 , @steakhal wrote:

> It seems like the checker is documented as `readability-data-pointer` but in 
> the tests it reports issues under the `readability-container-data-pointer` 
> name.
> Shouldn't they be the same? I think it will confuse the users.
> @aaron.ballman @whisperity @compnerd

Sounds like a typo or an intermittent rebase or rename missing something. 
Definitely should be consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

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

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D108893#3118389 , @whisperity 
wrote:

> In D108893#3111674 , @steakhal 
> wrote:
>
>> It seems like the checker is documented as `readability-data-pointer` but in 
>> the tests it reports issues under the `readability-container-data-pointer` 
>> name.
>> Shouldn't they be the same? I think it will confuse the users.
>
> Sounds like a typo or an intermittent rebase or rename missing something. 
> Definitely should be consistent.

@steakhal Fixed in rG164ee457a04daf727ead99b43ad490ea05523652 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893

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

[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.
This revision is now accepted and ready to land.

The fix is concise, thank you for observing and untangling the crash!

As the check was originally introduced in the ongoing release, we can go 
without a release notes entry.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:288
+protected:
+  virtual CONCAT(~Foo, Bar2()); // no-fixit
+};

It is strange that there is no fix-it here even though the keyword appears as a 
single token ...[1]



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:288
+protected:
+  virtual CONCAT(~Foo, Bar2()); // no-fixit
+};

whisperity wrote:
> It is strange that there is no fix-it here even though the keyword appears as 
> a single token ...[1]
Maybe a FIXME could be added for this case, just to register that we indeed 
realised something is //strange// here, but I'm not convinced neither for nor 
against. The purpose of the patch is to get rid of the crash, after all.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:299
+protected:
+  CONCAT(vir, tual) ~FooBar3();
+};

[1]... whereas here the check generates the fixit which removes the entire 
macro substitution!


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] D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D64454#3124423 , @carlosgalvezp 
wrote:

> I was a bit confused as well about the ClangSA support in clang-tidy. What's 
> the current status? Is clang-tidy running *all* checks from StaticAnalyzer?

If your package is built with CSA enabled, and you don't want to run //Cross 
Translation Unit// analysis mode, then yeah. After all, both Clang-Tidy and 
Clang Static Analyser are just "FrontendAction" libraries under the hood.



In D64454#3124312 , @aaron.ballman 
wrote:

> Yeah, I was worried this would get out of sycn and it really did not take 
> long for that to happen in practice. I think there's utility in listing the 
> checks in clang-tidy's documentation instead of pointing users to the SA 
> documentation page. Users should have one website to go to where they can see 
> what clang-tidy checks are available to them.

Does this still apply given that the quality and formatting match of the 
documentation website increased over the past one or two releases? The old 
website  if I see correctly no longer lists 
the checkers (and the previous format was also increasingly outdated and bad 
experience...), and we have the RST-based new website 
.

Come to think of it, maybe some RST hacks could automate "pulling in" the list 
from the (new) CSA website to the Tidy list, as long as maybe they are in a 
separate category (heading) in the Tidy list?
Although I'm unsure how much being `/tools/clang/` and 
`/tools/clang/tools/extra/` mess this thing up.
In case of Python, for example, there is a well-understood way of registering 
the documentation site of third-party packages, and //your// documentation 
generator can automatically generate the cross-reference that links to the 
other, from your doc's point-of-view, external website. But that involves a 
considerable amount of "magic".
As far as I know, Sphinx is written in Python, so one could write Python code 
that runs //during// the documentation generation (at least the RST part, I'm 
not sure about the `man` or `infopages` or `groff` stuff!) that can do "magic".



What we //COULD// do, however, is get rid of the individual checker 
documentation files (which all just contain machine generated redirects, at 
least according to this diff...) and have ALL the cross-referencing links 
(hand-written, autogenerated by our integration, or autogenerated by RST magic) 
point to not a page living inside Tidy's scope that just does a redirect, but 
to the appropriate heading in the (new) CSA doc suite?

(Then again, irrespective of what magic we will use, the non-trivial grouping 
structure on the new CSA docs site can become a problem...)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64454

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

[PATCH] D113863: [clang-tidy] Make `readability-container-data-pointer` more robust

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:20-23
+static const char ContainerExprName[] = "container-expr";
+static const char DerefContainerExprName[] = "deref-container-expr";
+static const char AddrofContainerExprName[] = "addrof-container-expr";
+static const char AddressOfName[] = "address-of";

Perhaps we could use `constexpr llvm::StringLiteral`?


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

https://reviews.llvm.org/D113863

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

[PATCH] D113848: [clang-tidy][NFC] Refactor ClangTidyDiagnosticConsumer files

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

What is the motivation behind the change? Can we be sure that these classes 
that were moved out weren't details to the context itself, and are 
understandable, usable, and can be relied upon without the context object?



In D113848#3130542 , @carlosgalvezp 
wrote:

> I agree with the comments, but I didn't want to touch any code other than 
> moving things around, since it's hard to see the changes in the diff 
> otherwise. I was also unsure if this was "LLVM convention" or a mistake. I'm 
> happy to fix in a separate patch if that's OK?

I think you can fix it now, LLVM style doesn't indent stuff that's within a 
namespace, so it won't change the meaningful part of the diff visually.




Comment at: clang-tools-extra/clang-tidy/ClangTidyContext.cpp:40-41
+#include 
+using namespace clang;
+using namespace tidy;
+

Eugene.Zelenko wrote:
> Shouldn't namespaces be used instead?
In general, you'd also want to make it more explicit by saying 

```
using namespace clang;
using namespace clang::tidy;
```

and not rely on precisely one `tidy` being visible from the previous set of 
available declarations.



Comment at: clang-tools-extra/clang-tidy/ClangTidyError.h:35-36
+
+} // end namespace tidy
+} // end namespace clang
+

Is this formatted properly? I thought the appropriate closing comment is `// 
namespace blah`, without the `end`. 😲 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113848

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

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:50
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), 
ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
+  int ErrorsDisplayed{0};
+  int ErrorsIgnoredCheckFilter{0};

carlosgalvezp wrote:
> salman-javed-nz wrote:
> > What's the prevalent style for class member initialization? `=` or `{}`?
> > cppcoreguidelines-prefer-member-initializer defaults to `{}` but I have 
> > seen both types in the code.
> I tried to find this info in the LLVM coding guidelines but didn't find 
> anything, so I assume it's maybe up to developers' discretion.
> 
> I prefer using braced initialization, since it prevents implicit conversions:
> https://godbolt.org/z/4sP4rGsrY
> 
> More strict guidelines like Autosar enforce this. Also CppCoreGuidelines 
> prefer that style as you point out.
I think this is such a new and within LLVM relatively unused feature (remember 
we are still pegged to C++14...) that we do not have a consensus on style, and 
perhaps warrants discussing it on the mailing list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113847

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

[PATCH] D113201: [clang-tidy] Fix a crash in modernize-loop-convert around conversion operators

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp:155-167
+/// Look through conversion/copy constructors and operators to find the 
explicit
 /// initialization expression, returning it is found.
 ///
 /// The main idea is that given
 ///   vector v;
 /// we consider either of these initializations
 ///   vector::iterator it = v.begin();

steakhal wrote:
> I think it's called //conversion member functions//.
Perhaps it would be nice if an example was added to the comment here which 
involves said conversion.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-basic.cpp:278
+  // conversion operator, which has no name, to Q::const_iterator.
+  Q Qt;
+  for (Q::const_iterator It = Qt.begin(), E = Qt.end(); It != E; ++It) {

This is not specific to //Qt//, the GUI framework, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113201

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

[PATCH] D113148: Add new clang-tidy check for string_view(nullptr)

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D113148#3109190 , @CJ-Johnson 
wrote:

> In D113148#3108993 , @aaron.ballman 
> wrote:
>
>> Generally speaking, we prefer to improve the existing checks. I think 
>> `bugprone-string-constructor` would probably be a better place for the 
>> constructor-related functionality. But that still means we don't have a good 
>> place for things like the assignment and comparison functionality, and it 
>> seems more useful to keep all of the `string_view`-with-`nullptr` logic in 
>> one place. That said, we should be careful we're not too onerous when users 
>> enable all `bugprone` checks together.
>
> As for "we should be careful we're not too onerous when users enable all 
> `bugprone` checks together.", these fixes are about preventing UB. While I 
> did put this in the "bugprone" module, perhaps "prone" is the wrong way to 
> think about it. It's unconditionally UB in all cases, not just a potential 
> bug. The suggested fixes are important for preventing crashes in the short 
> term, but more importantly for allowing the code to build in the future, 
> since the constructor overload `basic_string_view(nullptr_t) = delete;` is 
> being added.

@aaron.ballman Maybe we should do something to ease the burden of `bugprone-` 
becoming a catch-all basket, and create a new checker group for the 
"unconditional UB" or "in almost all cases this will make you do bad things" 
kinds of stuff.




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:82-90
+  Checks for various ways that the ``nullptr`` literal can be passed into the
+  ``const CharT*`` constructor of ``std::basic_string_view`` and replaces them
+  with the default constructor in most cases. For the comparison operators,
+  braced initializer list does not compile so instead the empty string is used.
+  Also, ``==`` and ``!=`` are replaced with calls to ``.empty()``.
+
+  This prevents code from invoking behavior which is unconditionally undefined.

Usually we only put a one sentence short summary into the release notes.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:13-14
+This prevents code from invoking behavior which is unconditionally undefined.
+The single-argument ``const char*`` constructor of ``std::string_view`` does
+not check for the null case before dereferencing its input.
+

You mentioned in the review discussion that a constructor taking 
`std::nullptr_t` will be added with an explicit `= delete;`. Maybe it is worth 
mentioning it here, if we have a standard version or a proposal on track for 
changing the STL like that?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:16-17
+
+.. code-block:: c++
+  std::string_view sv = nullptr;
+

Will this work? Isn't the `:` making RST confused here? You usually need an 
empty line because the block "instance" may be given additional options in RST.

It might compile the docs still, so this is the kind of thing that must be 
verified visually.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:46
+
+Note: The source pattern with trailing comment "A" selects the
+``(const CharT*)`` constructor overload and then value-initializes the pointer,

There should be a `.. note::` block which renders a nice box of note with an 
icon.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:49
+causing a null dereference. It happens to not include the ``nullptr`` literal,
+but it is still within the scope of this ClangTidy check.
+

Clang-Tidy. Or just simply "this check".



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst:53
+``(const CharT*, size_type)`` constructor which is perfectly valid, since the
+length argument is ``0``. It is not changed by this ClangTidy check.

ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113148

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

[PATCH] D111100: enable plugins for clang-tidy

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Wait... Is the diff //really// supposed to be this small? You didn't include 
code... that one header takes care of **everything**?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D00

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

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/GlobList.h:50-52
+/// A \p GlobList that caches search results, so that search is performed only
+/// once for the same query.
+class CachedGlobList {

If `CachedGlobList` //is-a// `GlobList`, wouldn't it be better to express the 
inheritance relationship?



Comment at: clang-tools-extra/clang-tidy/GlobList.h:61
+private:
+  GlobList Globs;
+  enum Tristate { None, Yes, No };

(considering the logically first data member is the conceptual base class)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113422

___
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 inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h:27
   SourceLocation LastReplacementEnd;
-  SourceRange LastTagDeclRange;
+  std::map LastTagDeclRanges;
+

Considering the pointer is just a number that hashes well, is there a reason to 
use the plain `std::map`? It is [[ 
https://llvm.org/docs/ProgrammersManual.html#map | generally discouraged ]].



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp:305-308
+
+typedef struct { int a; union { int b; }; } PR50990;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990 = struct { int a; union { int b; }; };

Just for the sake of testing a new logic, it would be beneficial to add a "more 
complex" nested example. At least one example where there are two sibling 
nests, and one where there is an additional level of nesting.


Repository:
  rG LLVM Github Monorepo

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] D112646: [clang-tidy] Add `readability-container-contains` check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:49
+
+  // Find containment checks which use `count`
+  
Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()),

Same comment about "membership" or "element" instead of "containment" here.



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:73
+
+  // Find inverted containment checks which use `count`
+  addSimpleMatcher(

Same comment about "membership" or "element" instead of "containment" here.



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:93
+
+  // Find containment checks based on `begin == end`
+  addSimpleMatcher(





Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:103
+void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) {
+  // Extract the ifnromation about the match
+  const auto *Call = Result.Nodes.getNodeAs("call");





Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:110
+
+  // Diagnose the issue
+  auto Diag =

I'm not sure if these comments are useful, though. The business logic flow of 
the implementation is straightforward.



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:113
+  diag(Call->getExprLoc(),
+   "use `contains` instead of `count` to check for containment");
+

This might be a bit nitpicking, but `containment` sounds off here: it usually 
comes up with regards to superset/subset relationships. Perhaps phrasing in 
`membership` or `element` somehow would ease this.



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:113
+  diag(Call->getExprLoc(),
+   "use `contains` instead of `count` to check for containment");
+

whisperity wrote:
> This might be a bit nitpicking, but `containment` sounds off here: it usually 
> comes up with regards to superset/subset relationships. Perhaps phrasing in 
> `membership` or `element` somehow would ease this.
We use single apostrophe (`'`) instead of backtick (`) in the diagnostics for 
symbol names.



Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h:19
+/// Finds usages of `container.count()` which should be replaced by a call
+/// to the `container.contains()` method introduced in C++ 20.
+///





Comment at: clang-tools-extra/docs/ReleaseNotes.rst:105-106
+
+  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.
+

Due to RST specifics, the code examples to be rendered as such, **double** 
backticks have to be used... Single backtick renders in a different font with 
emphasis (see example, blue), while double backtick is a proper monospace 
inline code snippet (see example, red).

{F20396099}



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:106
+  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.
+





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.
+

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.



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.
+

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.




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

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 o

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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D112646#3104236 , @avogelsgesang 
wrote:

> So I guess the name would have to be `container-count-begin-end-contains` or 
> similar... which would be a bit much in my opinion

Yeah, that would be too much indeed.

However, consider `readability-container-use-contains`. It indicates a bit more 
that we want people to write terse code that is clear about its intentions.


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 inline comments.



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;

`Comparison` instead of `Check`? These should be matching the `binaryOperator`, 
right?


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] D114105: [clang-tidy] Ignore narrowing conversions in case of bitfields

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:87
+  // Reading a small unsigned bitfield member will be wrapped by an implicit
+  // cast to 'int' triggering this checker. But this is known to be safe by the
+  // compiler since it chose 'int' instead of 'unsigned int' as the type of the

(Terminology... 😕)



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:100
+  // `-IntegerLiteral 'int' 1
+  const auto ShiftingWidenedBitfieldValue = castExpr(
+  hasCastKind(CK_IntegralCast), hasType(asString("int")),

`IntWidened`? Just to point out that so far this is the only case we've found 
where the warning is superfluous.


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

https://reviews.llvm.org/D114105

___
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 on

[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] 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] 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] 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 &M) {

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 g

[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] 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] 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] 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] 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(), &Invalid));
+Type = std::string(Lexer::getSourceText(
+CharSourceRange::getTokenRange(LastTagDeclRange->second),
+*Result.SourceManager, getLangOpts(), &Invalid));
 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] 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] 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] 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] 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 &operator=(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 &operator=(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] 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] 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
+++ clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck

[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 p

[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 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]

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] 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 &Itr,





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

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

[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] 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 &Ctx) {
+  ParentMapContext &PM = 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 &PM = 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.
+(void

[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] 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]

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&id=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 &K) const;
+  bool contains(const Key &K) const;
+  void *find(const Key &K);
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+};
+
+} // namespace std
+
+// Check that we detect various common ways to check for membership
+int testDifferentCheckTypes(std::map &MyMap) {
+  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 &MyMap) {
+  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 &M, std::unordered_set &US, std::set &S, std::multimap &MM) {
+  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);

[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] D111909: [clang-tidy] DefinitionsInHeadersCheck: Added option for checking C Code

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

It would be interesting to add a test for this. I've recently came across the 
fact that Clang doesn't support //common linkage// definitions, namely that in 
**C** (but not in C++), if you do the following:

  int I;
  void f1(void) {}



  int I;
  void f2(void) {}

and compile these two source files and link them together, the two `I`s will 
refer in the resulting binary to the same symbol. (You must not put an 
initialiser here!) Will `int I;` being in a header, with no initialiser, be 
caught?




Comment at: clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h:26
+///   - `CheckCCode`: Adds C99 to the minimum Language Requirements for this
+/// Checker. Disabled by default because this Checker wasn't build for C.
 ///   - `HeaderFileExtensions`: a semicolon-separated list of filename





Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-definitions-in-headers.rst:109
+   When `true` C99 is added to the minimum language requirements for this 
+   Cecker. Default is `false` because this Checker was not build for C.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111909

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

[PATCH] D111909: [clang-tidy] DefinitionsInHeadersCheck: Added option for checking C Code

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D111909#3070593 , @schrc3b6 wrote:

> I guess you don't want that to be cought if it is actually a tentative 
> definition. If I remember correctly for clang and gcc -fno-common is the 
> default.
> I think we could do one of two things here either create no warning if there 
> is a VarDelc without initialization or we could try to detect common linkage.
> I will have a look if I can detect a change from inside the checker if the 
> linkage of the variable changes via the fcommon compiler flag.

My personal belief after having encountered common linkage is that it is 
disgusting and it's good that Clang doesn't support them (by default).

But I do not have any stake or practical experience with working long term on 
//C// programs to have a well-educated opinion on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111909

___
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 inline comments.



Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:140
+  diag(InvocationParm->getLocation(),
+   "consider changing the %0st parameter of %1 from %2 to 'const %3 
&'",
+   DiagnosticIDs::Note)





Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:260
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' 
of the trivially-copyable type 'int' has no effect [performance-move-const-arg]
+  // CHECK-MESSAGES: :[[@LINE-9]]:28: note: consider changing the 2st 
parameter of showInt from 'int &&' to 'const int &'
+}




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] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

I think this is becoming okay and looks safe enough. I can't really grasp what 
`HasCheckedMoveSet` means, and how it synergises with storing `CallExpr`s so 
maybe a better name should be added. Do you mean `AlreadyCheckedMoves`? When is 
it possible that the same `CallExpr` of `std::move` will be matched multiple 
times?

In general going from `T&&` to `const T&` would be a broken change if the 
function implementation //calls// a non-const member method on the `T` 
instance, but I can be convinced that saying `consider [...]` instead of 
generating a bogus fix-it is enough, there is hopefully no need to extensively 
analyse and guard against this.

There has been a new release since the patch was proposed so a rebase should be 
needed.
@aaron.ballman What do you think, does this warrant a ReleaseNotes entry?


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] D112334: [clang-tidy] Suppress readability-static-access-through-instance for CUDA built-in variables

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D112334#3084533 , @aaron.ballman 
wrote:

> In D112334#3081213 , @carlosgalvezp 
> wrote:
>
>> Btw, regarding this `CHECK-MESSAGES-NOT`, how does it work? I can't find it 
>> in `check_clang_tidy.py`. Wouldn't the test fail anyway if unexpected 
>> warnings are printed?
>
> FileCheck matches input CHECK lines with output lines, so if there's no CHECK 
> line for a particular output, FileCheck won't report any problems with it. 
> (`-verify` is used by the frontend to verify diagnostics, but that's not 
> something that can be used within clang-tidy currently.) That said, I'm not 
> entirely certain of the mechanisms behind `CHECK-MESSAGES-NOT` in the python 
> scripts.

Just a clarification for the record, as I came across this too. There is no 
business logic per se associated with `CHECK-MESSAGES-NOT`. I think it's some 
sort of a bad example that we observed and taken from each other and sometimes 
overusing. `CHECK-MESSAGES-NOT` is useful in the context where there is a 
warning that currently isn't emitted (because the check isn't fully finished, 
or depends on a //FIXME// or something) but you **want** the message to appear 
later in the code. It indicates what should be there, but currently isn't there 
and we are not expecting it. The moment the check is further developed, it's 
easy to cut back the `-NOT` part (`2t-ld2w` if you're using Vim) and turn it 
into a proper check that //expects// the warning to be there. (And of course, 
hopefully also emits it!)

Semantically, it is equivalent to saying `// NO-WARN: Clang is awesome!` or `// 
MY-LITTLE-PONY` or whatever in the text. It will be ignored by the script. 😉  
The script only triggers for `CHECK-MESSAGES:`, `CHECK-FIXES:` and 
`CHECK-NOTES:` and the prefixes you may or may not be able to specify at 
invocation explicitly.

IMHO if we never want that warning to pop it is misleading and noise to add the 
would-be text of a warning there. If you want to indicate that there is no need 
for a warning (any warning emitted by the check!), I believe it is better to 
use `// NO-WARN`. That indicates both the intent that the code in that line or 
nearby //SHOULD PASS// but also clearly documents that it wasn't a developer 
oversight that the test appears as a "negative case" (i.e. you did not 
//forget// to add the `CHECK-` lines in).


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

https://reviews.llvm.org/D112334

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

[PATCH] D112334: [clang-tidy] Suppress readability-static-access-through-instance for CUDA built-in variables

[Whisperity via Phabricator via cfe-commits]

whisperity closed this revision.
whisperity added a comment.

@carlosgalvezp Force pushes are prevented by the serverside integration so we 
couldn't force push even if we wanted to. What one could do is to revert the 
patch and land it again (or land an empty patch that has the association line 
in the message...) but both of those would be just excess noise in the (already 
wy too noisy!) history.

Hopefully this will help.


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

https://reviews.llvm.org/D112334

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

[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

[Whisperity via Phabricator via cfe-commits]

whisperity added reviewers: martong, steakhal.
whisperity added a subscriber: balazske.
whisperity added a comment.
Herald added subscribers: manas, ASDenysPetrov, donat.nagy.

Pinging my people here, perhaps you being more knowledgeable in the analyser 
can say if this patch is salvageable in any way? Or did the state of the art 
moved past this in the years that went by? The patch itself doesn't look huge, 
but outside my direct expertise.


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

https://reviews.llvm.org/D46081

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

[PATCH] D112596: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

100% typo. 😂 Aww 'chute... Bunch of rebases.
Yeah I didn't add extensive tests for simply the configuration, only for the 
configurations working.

Did you check the documentation file? It should mention the default values for 
the checker configuration options. Does it say `ForwardIt` there properly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112596

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

[PATCH] D112596: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters

[Whisperity via Phabricator via cfe-commits]

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

Perfect! Have you obtained commit rights already, or should I go ahead and 
commit this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112596

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

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:45-50
+// The following functions are
+// deliberately excluded because they can
+// be called with NULL argument and in
+// this case the check is not applicable:
+// mblen, mbrlen, mbrtowc, mbtowc, wctomb,
+// wctomb_s

aaron.ballman wrote:
> Pretty sure this comment can be re-flowed to 80 columns. Also needs trailing 
> punctuation.
Shouldn't we reuse `utils::options::serializeStringList` here instead of 
hardcoding the separator character into a giant literal? I know executing that 
operation has an associated run-time cost, and as it is not `constexpr` it 
needs to be done somewhere else, and `std::string` could throw so we can't do 
it at "static initialisation" time... But having those extra chars there just 
seems way too fragile at a later modification. We've had cases where people 
missed separating `,`s even -- and those are syntactically highlighted 
differently due to being outside the string literal.

Suggestion:

 * An array of `StringRef`s or even `llvm::StringLiteral`s containing the 
individual function names. Array separated with `,`, the separator outside the 
literal. Aligned to column and probably one per line.
 * When using this variable later (🌟), instead of passing the stringref, pass 
the result of `serializeStringList`.



Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:325
 Opts["cert-str34-c.DiagnoseSignedUnsignedCharComparisons"] = "false";
+Opts["cert-err33-c.CheckedFunctions"] = CertErr33CCheckedFunctions;
 return Options;

(🌟)



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-err33-c.rst:11
+
+* aligned_alloc()
+* asctime_s()

`mblen`, `mbrlen`, etc. are with backticks later. But this list isn't. Was this 
intended?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112409

___
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 reviewers: aaron.ballman, xazax.hun.
whisperity added a comment.
Herald added a subscriber: rnkovacs.

Why `readability-`, if the intent is to make users move to a newer API?

@xazax.hun I think you did something similar wrt. `empty()`, right? Could you 
take a look at this?




Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:341-343
+   `cert-exp42-c `_, `bugprone-suspicious-memory-comparison 
`_,
`cert-fio38-c `_, `misc-non-copyable-objects 
`_,
+   `cert-flp37-c `_, `bugprone-suspicious-memory-comparison 
`_,

These are unrelated changes coming from a lack of rebase or a too eager diff.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst:15
+---  --
+``x.begin() == x.end()``   ``!myMap.contains(x)``
+``x.begin() != x.end()``   ``myMap.contains(x)``

The indentation looks offset for the 2nd column here.


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] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Okay. I think I am convinced. And removing a bogus automated fix is always a 
positive change!




Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:144
+  << (InvocationParm->getFunctionScopeIndex() + 1)
+  << ReceivingCallExpr->getDirectCallee()->getName()
+  << InvocationParm->getOriginalType() << ExpectParmTypeName;

I am not sure if this will always return something reasonable as a printed name.

Could you please add a test case where the receiving function is a 
//constructor//? E.g. a `testStringView` that takes, let's say, `const char*&&`?



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:116-119
+- Improved :doc:`performance-move-const-arg` check.
+
+  Removed fixes for that a trivially-copyable object wrapped by std::move is 
passed to the function with rvalue reference parameters because the code was 
broken by removing std::move.
+

I tried to make the release note entry a bit easier to understand. Of course, 
please reflow!


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] D112730: [clang-tidy] Add AUTOSAR module

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

"C++14 Guidelines"? So it's always and definitely C++14 specific? What is the 
licencing approach of this guideline? Looking up with searchers seems to turn 
up a few PDFs which say `--- AUTOSAR CONFIDENTIAL ---`, yet they are up and out 
on the official-looking website.




Comment at: clang-tools-extra/clang-tidy/autosar/CMakeLists.txt:26
+  clangTooling
+  clangStaticAnalyzerCheckers
+  )

I'm not sure this is needed here, for this module, as of now.


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

https://reviews.llvm.org/D112730

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

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables by removing the enum FixIt, and add support for initialization check of scoped enum.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@Sockke Sorry to circle back here, but it seems to me that Clang now has a flag 
`-Wuninitialized`. Could you check how it behaves, compared to this check? If 
there are overlaps, what should we do, @aaron.ballman? Should parts of the 
check be deprecated?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106431

___
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.

I'm a bit confused about this, but it has been a while since I read this patch. 
Am I right to think that the code in the patch and the original submission (the 
patch summary) diverged since the review started? I do not see anything 
"removed" from the test files, only a new addition. Or did you accidentally 
upload a wrong diff somewhere? The original intention of the patch was to 
remove bogus printouts.

In general: the test file ends without any testing for functions with multiple 
parameters. Are they out-of-scope by design?




Comment at: 
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:131-145
 auto Diag = diag(FileMoveRange.getBegin(),
  "std::move of the %select{|const }0"
- "%select{expression|variable %4}1 "
- "%select{|of the trivially-copyable type %5 }2"
- "has no effect; remove std::move()"
- "%select{| or make the variable non-const}3")
+ "%select{expression|variable %5}1 "
+ "%select{|of the trivially-copyable type %6 }2"
+ "has no effect%select{; remove std::move()||; consider "
+ "changing %7's parameter from %8 to 'const %9 &'}3"
+ "%select{| or make the variable non-const}4")





Comment at: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp:139
 << IsConstArg << IsVariable << IsTriviallyCopyable
-<< (IsConstArg && IsVariable && !IsTriviallyCopyable) << Var
-<< Arg->getType();
+<< (IsRValueReferenceArg + (IsRValueReferenceArg &&
+IsTriviallyCopyable &&

**`+`**? Shouldn't this be `||`? I know they're equivalent over Z_2, but if 
we're using `&&` instead of `*` then let's adhere to the style.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:262-263
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' 
of the trivially-copyable type 'int' has no effect
+}

Quuxplusone wrote:
> Sockke wrote:
> > Quuxplusone wrote:
> > > ```
> > >   forwardToShowInt(std::move(a));
> > >   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 
> > > 'a' of the trivially-copyable type 'int' has no effect
> > > ```
> > > I continue to want-not-to-block this PR, since I think it's improving the 
> > > situation. However, FWIW...
> > > It's good that this message doesn't contain a fixit, since there's 
> > > nothing the programmer can really do to "fix" this call. But then, should 
> > > this message be emitted at all? If this were `clang -Wall`, then we 
> > > definitely would //not// want to emit a "noisy" warning where there's 
> > > basically nothing the programmer can do about it. Does clang-tidy have a 
> > > similar philosophy? Or is it okay for clang-tidy to say "This code looks 
> > > wonky" even when there's no obvious way to fix it?
> > > ```
> > >   forwardToShowInt(std::move(a));
> > >   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 
> > > 'a' of the trivially-copyable type 'int' has no effect
> > > ```
> > > I continue to want-not-to-block this PR, since I think it's improving the 
> > > situation. However, FWIW...
> > > It's good that this message doesn't contain a fixit, since there's 
> > > nothing the programmer can really do to "fix" this call. But then, should 
> > > this message be emitted at all? If this were `clang -Wall`, then we 
> > > definitely would //not// want to emit a "noisy" warning where there's 
> > > basically nothing the programmer can do about it. Does clang-tidy have a 
> > > similar philosophy? Or is it okay for clang-tidy to say "This code looks 
> > > wonky" even when there's no obvious way to fix it?
> > 
> > Yes, I agree with you. I did not remove warning to maintain the original 
> > behavior, which will make the current patch clearer. In any case, it is 
> > easy for me to make this modification if you want. 
> I'll defer on this subject to whomever you get to review the code change in 
> `MoveConstArgCheck.cpp`. (@whisperity perhaps?)
(Negative, it would be @aaron.ballman or @alexfh or the other maintainers. I'm 
just trying to give back to the community after taking away //a lot// of 
reviewer effort with a humongous check that I just recently pushed in after 
//years// of (re-)development.)



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:262-263
+  int a = 10;
+  forwardToShowInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' 
of the trivially-copyable type 'int' has no effect
+}

whisperity wrote:
> Quuxplusone wrote:
> > Sockke wr

[PATCH] D108808: [clang-tidy] bugprone-infinite-loop: Fix false positives with volatile addresses.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

What happens for something like the following? Maybe it's also worth a test 
case?

  bool wait_analogue_pin(volatile int* address, int threshold) {
while (*address < threshold) {}
return true;
  }


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D108808

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

[PATCH] D108808: [clang-tidy] bugprone-infinite-loop: Fix false positives with volatile addresses.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D108808#2968742 , @gribozavr2 
wrote:

> In D108808#2968721 , @whisperity 
> wrote:
>
>> What happens for something like the following? Maybe it's also worth a test 
>> case?
>
> WDYT about the test on line 614?

Ah! The fact that the actual value was there misled me.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D108808

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

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D102325#2970683 , @mgartmann wrote:

> @aaron.ballman Thanks a lot for your review!
>
> Can somebody help me merging this change? I do not have the rights to do so.
> Thanks in advanve!

Sure, just please specify what name and e-mail address you'd like to have 
associated with the commit. Ever since we moved to Git, now we have a 
"built-in" way of distinguishing the two fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102325

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

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Uuuh.. I get the sentiment, but this change breaks the very essence of the joke 
that the default generated code wants to pass on. It also does not showcase 
that we can emit notes, not just warnings.

How about `function %0 is insufficiently awesome, mark it as 'awesome_'!` in 
the warning text? (Note how the fix and the note's text diverged already.)

And so we should come up with a joke for the Note tag.

BTW, the test wasn't checking for the note's message at all?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109210

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

[PATCH] D108912: [release][analyzer] Add 13.0.0 release notes

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:300-304
+.. 90377308de6c [analyzer] Support allocClassWithName in OSObjectCStyleCast 
checker
+
+- Add support for ``allocClassWithName`` in OSObjectCStyleCast checker.
+
+.. cad9b7f708e2b2d19d7890494980c5e427d6d4ea: Print time taken to analyze each 
function

Why is the commit prefix different between these two?


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

https://reviews.llvm.org/D108912

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

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@mattbeardsley Thank you for the explanation. (I'd wager some of your 
observations could even be added to some developer docs that's versioned in the 
repo. 😉) I'm not opposed to the changes at all. And indeed, it looks like 
`Note`s are really an advanced feature. I'm incredibly biased because I just 
recently finished the development of a checker in which I extensively use notes 
(in some test cases, there were like 8 or 9 notes attached to a warning, albeit 
all without a `FixIt` attached).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109210

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

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D102325#2981760 , @mgartmann wrote:

> @whisperity Thanks a lot for helping me out!

Sorry I got busy with a few official business and then got deeply distracted by 
what I'm working on, but I'll get to committing this ASAP. ETA 1 hr. 🙂


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102325

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

[PATCH] D102325: [clang-tidy] cppcoreguidelines-virtual-base-class-destructor: a new check

[Whisperity via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc58c7a6ea053: [clang-tidy] 
cppcoreguidelines-virtual-base-class-destructor: a new check (authored by 
mgartmann, committed by whisperity).

Changed prior to commit:
  https://reviews.llvm.org/D102325?vs=365161&id=371551#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102325

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-virtual-class-destructor.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -0,0 +1,204 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-virtual-class-destructor %t -- --fix-notes
+
+// CHECK-MESSAGES: :[[@LINE+4]]:8: warning: destructor of 'PrivateVirtualBaseStruct' is private and prevents using the type [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+3]]:8: note: make it public and virtual
+// CHECK-MESSAGES: :[[@LINE+2]]:8: note: make it protected
+// As we have 2 conflicting fixes in notes, no fix is applied.
+struct PrivateVirtualBaseStruct {
+  virtual void f();
+
+private:
+  virtual ~PrivateVirtualBaseStruct() {}
+};
+
+struct PublicVirtualBaseStruct { // OK
+  virtual void f();
+  virtual ~PublicVirtualBaseStruct() {}
+};
+
+// CHECK-MESSAGES: :[[@LINE+2]]:8: warning: destructor of 'ProtectedVirtualBaseStruct' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:8: note: make it protected and non-virtual
+struct ProtectedVirtualBaseStruct {
+  virtual void f();
+
+protected:
+  virtual ~ProtectedVirtualBaseStruct() {}
+  // CHECK-FIXES: ~ProtectedVirtualBaseStruct() {}
+};
+
+// CHECK-MESSAGES: :[[@LINE+2]]:8: warning: destructor of 'ProtectedVirtualDefaultBaseStruct' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:8: note: make it protected and non-virtual
+struct ProtectedVirtualDefaultBaseStruct {
+  virtual void f();
+
+protected:
+  virtual ~ProtectedVirtualDefaultBaseStruct() = default;
+  // CHECK-FIXES: ~ProtectedVirtualDefaultBaseStruct() = default;
+};
+
+// CHECK-MESSAGES: :[[@LINE+4]]:8: warning: destructor of 'PrivateNonVirtualBaseStruct' is private and prevents using the type [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+3]]:8: note: make it public and virtual
+// CHECK-MESSAGES: :[[@LINE+2]]:8: note: make it protected
+// As we have 2 conflicting fixes in notes, no fix is applied.
+struct PrivateNonVirtualBaseStruct {
+  virtual void f();
+
+private:
+  ~PrivateNonVirtualBaseStruct() {}
+};
+
+// CHECK-MESSAGES: :[[@LINE+2]]:8: warning: destructor of 'PublicNonVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:8: note: make it public and virtual
+struct PublicNonVirtualBaseStruct {
+  virtual void f();
+  ~PublicNonVirtualBaseStruct() {}
+  // CHECK-FIXES: virtual ~PublicNonVirtualBaseStruct() {}
+};
+
+struct PublicNonVirtualNonBaseStruct { // OK according to C.35, since this struct does not have any virtual methods.
+  void f();
+  ~PublicNonVirtualNonBaseStruct() {}
+};
+
+// CHECK-MESSAGES: :[[@LINE+4]]:8: warning: destructor of 'PublicImplicitNonVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+3]]:8: note: make it public and virtual
+// CHECK-FIXES: struct PublicImplicitNonVirtualBaseStruct {
+// CHECK-FIXES-NEXT: virtual ~PublicImplicitNonVirtualBaseStruct() = default;
+struct PublicImplicitNonVirtualBaseStruct {
+  virtual void f();
+};
+
+// CHECK-MESSAGES: :[[@LINE+5]]:8: warning: destructor of 'PublicASImplicitNonVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+4]]:8: note: make it public and virtual
+// CHECK-FIXES: struct PublicASImplicitNonVirtualBaseStruct {
+// CHECK-FIXES-NEXT: virtual ~PublicASImplicitNonVirtualBaseStruct() = default;
+// CHECK-FIXES-NEXT: private:
+struct PublicASImplicitNonVirtualBaseStruct {
+private:
+  virtual void f();
+};
+
+struct ProtectedNonVirtualBaseStruct { // OK
+  virtual void f();
+
+protected:
+  ~ProtectedNonVirtualBaseStruct() {}
+};
+
+//

[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> &s) {
>^
>   
> /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] 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 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