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

Whew, this is a big one. Generally looks good, although I would separate 
implementation detail functions a bit better, with perhaps more comments to 
move them apart a bit, it is really harsh to scroll through.

Could you please re-run the findings on the projects? There has been a lot of 
improvements to the checker.
I think we should review it as it is now, and do further enhancements later on, 
in subsequent patches, where the enhancement implementations themselves are 
revealed better in the diff.


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

Generally grammar / phrasing things that have caught my eye.

The rest of the code looks okay, bar my previous comment about better 
commenting / separating chunks of helper functions.




Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:74
+ const MatchFinder::MatchResult &Result) {
+  return Lexer::getLocForEndOfToken(E->getLocEnd(), 0, *Result.SourceManager,
+Result.Context->getLangOpts());

`getLocEnd` will be removed, see D50353



Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:294
+  Diag << RHSRemoveFix;
+else if (LengthExpr->getLocEnd() == InnerOpExpr->getLocEnd())
+  Diag << LHSRemoveFix;

`getLocEnd` and `getLocStart` used here too.



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:42
+
+Transformation rules with 'memcpy()'
+

with -> of



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:46
+It is possible to rewrite the ``memcpy()`` and ``memcpy_s()`` calls as the
+following four function:  ``strcpy()``, ``strncpy()``, ``strcpy_s()``,
+``strncpy_s()``, where the latter two is the safe version. Analogly it is

function**s**



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:47
+following four function:  ``strcpy()``, ``strncpy()``, ``strcpy_s()``,
+``strncpy_s()``, where the latter two is the safe version. Analogly it is
+possible to rewrite ``wmemcpy()`` related functions handled in the same way.

are, versions

(Perhaps as this is user-facing code a half sentence about what safe version 
means could also be put here.)



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:47
+following four function:  ``strcpy()``, ``strncpy()``, ``strcpy_s()``,
+``strncpy_s()``, where the latter two is the safe version. Analogly it is
+possible to rewrite ``wmemcpy()`` related functions handled in the same way.

whisperity wrote:
> are, versions
> 
> (Perhaps as this is user-facing code a half sentence about what safe version 
> means could also be put here.)
Analogly -> Respectively, / In a similar fashion,



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:48
+``strncpy_s()``, where the latter two is the safe version. Analogly it is
+possible to rewrite ``wmemcpy()`` related functions handled in the same way.
+

``-related
~~handled~~



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:52
+
+- If the type of the destination array is not just ``char``, that means it 
+  cannot be any string handler function. But if the given length is

What is "just char"? Is `unsigned char` or `signed char` not "just char"?



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:52-53
+
+- If the type of the destination array is not just ``char``, that means it 
+  cannot be any string handler function. But if the given length is
+  ``strlen(source)`` then fix-it adds ``+ 1`` to it, so the result will be

whisperity wrote:
> What is "just char"? Is `unsigned char` or `signed char` not "just char"?
"that means it cannot be any string handler function" This sentence doesn't 
make any sense.



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:54
+  cannot be any string handler function. But if the given length is
+  ``strlen(source)`` then fix-it adds ``+ 1`` to it, so the result will be
+  null-terminated.

Will be, or can be? Simply allocating more memory won't magically make a null 
terminator happen, I think. (Although I can be wrong.)



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:59
+
+- If the destination cannot overflow then the new function is should be the 
+  simple ``cpy()``, because it is more efficient.

The destination by itself can't overflow as it is a properly allocated memory.

Perhaps you meant "If copy to the destination array cannot overflow"?



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:59
+
+- If the destination cannot overflow then the new function is should be the 
+  simple ``cpy()``, because it is more efficient.

whisperity wrote:
> The destination by itself can't overflow as it is a properly allocated memory.
> 
> Perhaps you meant "If copy to the destination array cannot overflow"?
... then the simple `cpy()`-like functions should be used as they are more 
efficient.

(~~is should be the~~)



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.r

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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Minor comments in-line, looking good on first glance.

I'm not sure if we discussed, has this checker been tried on some in-the-wild 
examples? To see if there are some real findings or crashes?
There is a good selection of projects here in this tool: 
https://github.com/Xazax-hun/csa-testbench.




Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:46
+};
+}
+

Szelethus wrote:
> ` // end of anonymous namespace`
This comment can be marked as //Done//.



Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:52
+  llvm::raw_string_ostream OS(Diagnostics);
+  OS << "Sorting pointer-like keys can result in non-deterministic ordering";
+

I generally have a concern about calling these thing "keys". How did you mean 
this? The file's top comment says //elements//.



Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:81-87
+ callsName("std::is_sorted"),
+ callsName("std::nth_element"),
+ callsName("std::partial_sort"),
+ callsName("std::sort"),
+ callsName("std::stable_sort"),
+ callsName("std::partition"),
+ callsName("std::stable_partition")

Please order this listing.



Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:102
+  auto Matches = match(MatcherM, *D, AM.getASTContext());
+  for (BoundNodes Match : Matches)
+emitDiagnostics(Match, D, BR, AM, this);

`emitDiagnostics` takes a `const T&`. To prevent unnecessary copy operations in 
this iteration, you should also use `const BoundNode &Match`.


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 project: clang.
whisperity added a comment.

@Szelethus `clang-query` seems to sometimes not include matcher functions that 
are perfectly available in the code... I recently had some issue with using 
`isUserDefined()`, it was available in my code, but `clang-query` always 
rejected it. Seems there isn't an automatic 1:1 mapping between the two.

@NoQ What is your decision about introducing a new `NonDeterminism` group? 
Should be, shan't be, is this name right?


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] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Ping.
Shouldn't let this thing go to waste.


https://reviews.llvm.org/D46081



___
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 subscriber: martong.
whisperity added a comment.

Make sure you use only the C++ projects like BitCoin, LLVM/Clang, ProtoBuf and 
such from the Xazax CSA Test suite because this checker is not really 
applicable to C projects.

Generally I like the idea (personally I've ran into a similar issue to ordering 
of sets in Java missed and on another OS we failed the tests where they tried 
to stringify elements and compare the output...) very much, this is one of 
those subleties you can really miss when writing and when reviewing code, and 
it will only bite you in the ass months down the line.

I think I'll summon @martong here, //ASTMatchers// are not my fortée, but he 
has been tinkering with them much recently.




Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:108-110
+void ento::registerPointerSortingChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker();
+}

Speaking of not being applicable to C code, I think this snippet here should 
allow you to skip running the checker if you are not on a C++ file:

```
if (!Mgr.getLangOpts().CPlusPlus)
  return;
```


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 elements

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In https://reviews.llvm.org/D50488#1225064, @george.karpenkov wrote:

> Why explicitly skip C  projects? We would not be able to match `std::X` 
> functions anyway.


Why spend time constructing matchers and running on the AST when we are sure 
that generating any result is impossible? Skipping the entire useless traversal 
of the tree can end up saving precious execution time that should not be 
overlooked.


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

https://reviews.llvm.org/D50353 has landed, so after a rebase this patch will 
not compile.


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] D51926: [scan-build-py] Prevent crashes of CTU analysis by suppressing warnings

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Will this properly synergise across compilers with user-specified warning 
options, such as `-Wall -Werror`?


Repository:
  rC Clang

https://reviews.llvm.org/D51926



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

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Softly pinging this. Perhaps we could discuss this and get it in before Clang7 
rides off into the sunset?


https://reviews.llvm.org/D45094



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

[PATCH] D45095: [clang-tidy] Align usage of ClangTool interface with new VFS injection

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Akin to https://reviews.llvm.org/D45094, pinging this too. 🙂


https://reviews.llvm.org/D45095



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

[PATCH] D49228: [analyzer][UninitializedObjectChecker] Void pointer objects are casted back to their dynmic type in note message

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: test/Analysis/cxx-uninitialized-object-ptr-ref.cpp:290
 struct IntDynTypedVoidPointerTest1 {
-  void *vptr; // expected-note{{uninitialized pointee 'this->vptr'}}
+  void *vptr; // expected-note{{uninitialized pointee 'this->static_cast(vptr)'}}
   int dontGetFilteredByNonPedanticMode = 0;

NoQ wrote:
> Shouldn't this rather say something like `static_cast(this->vptr)`? 
> That's the normal syntax to do this sort of stuff, i guess.
Not just "normal syntax", this syntax of the output does not compile.

```
foo.cpp:6:13: error: expected unqualified-id
  this->static_cast(vptr) = new int();
```


https://reviews.llvm.org/D49228



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

[PATCH] D43120: [clang-tidy] New checker for exceptions that are created but not thrown

[Whisperity via Phabricator via cfe-commits]

whisperity requested changes to this revision.
whisperity added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/misc/ThrowKeywordMissingCheck.cpp:21
+void ThrowKeywordMissingCheck::registerMatchers(MatchFinder *Finder) {
+  // This is a C++ only checker.
+  if (!getLangOpts().CPlusPlus)

Superfluous comment, this can be removed.



Comment at: clang-tidy/misc/ThrowKeywordMissingCheck.cpp:32
+   hasType(cxxRecordDecl(
+   isSameOrDerivedFrom(matchesName("[Ee]xception|EXCEPTION",
+   unless(anyOf(hasAncestor(stmt(

`matchesName` says

> Does not match typedefs of an underlying type with the given name.

Does your check find the following case?

```
typedef std::exception ERROR_BASE;
class my_error : public ERROR_BASE {
  /* Usual yadda. */
};

int main() {
  my_error();
}
```




Comment at: clang-tidy/misc/ThrowKeywordMissingCheck.cpp:46
+  Result.Nodes.getNodeAs("temporary-exception-not-thrown");
+  diag(TemporaryExpr->getLocStart(), "exception created but is not thrown");
+}

Either use passive for both subsentences or don't use passive at all. Maybe one 
of the following could be a better wording:

> exception instantied and not thrown

> exception is constructed but is not thrown

> exception construction without a throw statement



Comment at: test/clang-tidy/misc-throw-keyword-missing.cpp:23
+// std::exception and std::runtime_error declaration
+struct exception{
+  exception();

Please format the code. Why isn't there a space before `{`?



Comment at: test/clang-tidy/misc-throw-keyword-missing.cpp:29
+
+struct runtime_error : public exception{
+  explicit runtime_error( const std::string& what_arg );

Format here too.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43120



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

[PATCH] D43120: [clang-tidy] New checker for exceptions that are created but not thrown

[Whisperity via Phabricator via cfe-commits]

whisperity resigned from this revision.
whisperity added a comment.

Works for me but I haven't any sayings in these. 😇


https://reviews.llvm.org/D43120



___
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 or wcslen

[Whisperity via Phabricator via cfe-commits]

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

In general, make sure the documentation page renders well in a browser.

Mostly style and phrasing stuff inline:




Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.h:34-36
+  // If non-zero the target version implements _s suffixed memory and character
+  // handler functions which is safer than older versions (e.g. 'memcpy_s()').
+  const int IsSafeFunctionsAreAvailable;

More of a language or phrasing thing, but the singular/plural wording is 
anything but matched in this case: handler functions which **are** safer. What 
is a "target version" in this case? Shouldn't it be something like "target 
environment" or "target standard" or just simply "target"?

The variable name is also problematic. `AreSafeFunctionsAvailable` would be 
better.



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:20-21
+the passed third argument, which is ``size_t length``. The proper length is
+``strlen(src) + 1`` because the null terminator need an extra space. The result
+is badly not null-terminated:
+

//badly?// Also, perhaps a half sentence of explanation would be nice here:

need an extra space, thus the result is not null-terminated, which can result 
in undefined behaviour when the string is read.



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:39
+
+Otherwise fix-it will rewrite it to a safer function, that born before ``_s``
+suffixed functions:

That //existed//.



Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:105
+
+   An integer non-zero value specifying if the target version implements ``_s``
+   suffixed memory and character handler functions which is safer than older

Why is this an integer, rather than a bool?


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] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Whisperity via Phabricator via cfe-commits]

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

Ah, and the function names in the test files have been made more logical.

Let's roll. 😉


https://reviews.llvm.org/D45532



___
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] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:420-465
 if (ChecksEnabled[CK_InvalidatedIteratorChecker] &&
 isAccessOperator(Func->getOverloadedOperator())) {
   // Check for any kind of access of invalidated iterator positions
   if (const auto *InstCall = dyn_cast(&Call)) {
 verifyAccess(C, InstCall->getCXXThisVal());
   } else {
 verifyAccess(C, Call.getArgSVal(0));

This is a bit of an organisational comment (and the compiler of course 
hopefully optimises this out), but could you, for the sake of code readability, 
break the if-elseif-elseif-elseif chain's common check out into an outer if, 
and only check for the inner parts? Thinking of something like this:

```
if (ChecksEnabled[CK_InvalidatedIteratorChecker])
{
   /* yadda */
}

if (ChecksEnabled[CK_IteratorRangeChecker])
{
  X* OOperator = Func->getOverloadedOperator();
  if (isIncrementOperator(OOperator))
  {
/* yadda */
  } else if (isDecrementOperator(OOperator)) {
/* yadda */
  }
}

/* etc. */ 
```



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1074
 
-  auto &SymMgr = C.getSymbolManager();
-  auto &SVB = C.getSValBuilder();
-  auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
-  const auto OldOffset = Pos->getOffset();
-  const auto intValue = value.getAs();
-  if (!intValue)
+  // Incremention or decremention by 0 is never bug
+  if (isZero(State, Value.castAs()))

is never a, also `.` at the end



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1568
 
+IteratorPosition IteratorChecker::shiftPosition(CheckerContext &C,
+OverloadedOperatorKind Op,

(Minor: I don't like the name of this function, `advancePosition` (akin to 
`std::advance`) sounds cleaner to my ears.)



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1575
+  auto &SVB = C.getSValBuilder();
+  auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
+  if (const auto IntDist = Distance.getAs()) {

For the sake of clarity, I think an assert should be introduced her, lest 
someone ends up shifting the position with `<=`.



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:2329-2330
 
   // Out of range means less than the begin symbol or greater or equal to the
   // end symbol.
 

How does the introduction of `IncludeEnd` change this behaviour? What does the 
parameter refer to?


Repository:
  rC Clang

https://reviews.llvm.org/D53812



___
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 reviewer: dyung.
whisperity added a subscriber: dyung.
whisperity added a comment.

In https://reviews.llvm.org/D45050#1280831, @dyung wrote:

> I have attached a bzipped preprocessed file that I can confirm will show the 
> failure if built with clang++ version 3.7.1, specifically the version that 
> shipped with ubuntu 14.04.5 LTS. Here is the full version information:
>
> Ubuntu clang version 3.7.1-svn253742-1~exp1 (branches/release_37) (based on 
> LLVM 3.7.1)
>
> Target: x86_64-pc-linux-gnu
>  Thread model: posix
>
> If you build it with “clang++ -std=c++11 
> NotNullTerminatedResultCheck.preproc.cpp” you should see the failure.


I have installed said Ubuntu in a virtual machine for testing this, but 
unfortunately only the following Clangs are available in the package manager 
for `Trusty`:

  clang - C, C++ and Objective-C compiler (LLVM based)
  clang-3.3 - C, C++ and Objective-C compiler (LLVM based)
  clang-3.4 - C, C++ and Objective-C compiler (LLVM based)
  clang-3.5 - C, C++ and Objective-C compiler (LLVM based)
  clang-3.6 - C, C++ and Objective-C compiler (LLVM based)
  clang-3.8 - C, C++ and Objective-C compiler (LLVM based)
  clang-3.9 - C, C++ and Objective-C compiler (LLVM based)

(Where `clang` is just a synonym for `clang-3.4`.) **There is no Clang 3.7 in 
the package upstream, it seems.**

--

However, **`16.04 LTS (Xenial)`** at the time of writing this comment has an 
`clang-3.7` package, specifically this version:

  Ubuntu clang version 3.7.1-2ubuntu2 (tags/RELEASE_371/final) (based on LLVM 
3.7.1)
  Target: x86_64-pc-linux-gnu
  Thread model: posix

With this I can confirm I get a huge trace and the template depth overflow 
failure.

However, from the filename-line mappings of the preprocessed output, I can see 
that the `type_traits` header comes from `/usr/include/c++/4.8/type_traits`, 
which is a version **4.8** standard library, but installing the `clang-3.7` 
package (through some transitivity in `libasan` and such) depended on 
`gcc-`**`5`**`-base`, upgrading it from the system-default `5.3.1` to `5.4.0`.

Isn't this a discrepancy, relying on an older standard library than what is 
seemingly available on the system?


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] D35068: [analyzer] Detect usages of unsafe I/O functions

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D35068#1361902 , @Szelethus wrote:

> Edit: it doesn't, but CMake is mostly a C project and it does!


CMake really isn't a C project if you look at what language it actually uses - 
the C files come from tests and system utilities such as ZIP being 
reimplemented.
And as far as I've seen with my recent endeavours, Clang has problems with 
parsing and building CMake (however all these errors relate to some very niche 
standard library memory size related stuff not being there where it wants them).


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

https://reviews.llvm.org/D35068



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



Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:18
+This for loop is an infinite loop because the short type can't represent all
+values in the [0..size] interval.
+

Format the interval as code (monospace): `[0 .. size]`



Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:24
+
+int doSometinhg(const std::vector& items) {
+for (short i = 0; i < items.size(); ++i) {}

There is a typo in the function's name.



Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:116
+
+// TODO: handle those cases when the loop condition contains a floating point 
expression
+void voidDoubleForCond() {

(Misc: You might want to check out D52730 for floats later down the line.)


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] D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1018
+auto It = CurrExpArgTokens.begin();
+while (It != CurrExpArgTokens.end()) {
+  if (It->isNot(tok::identifier)) {

xazax.hun wrote:
> Maybe a for loop mor natural here?
I asked this one already in the earlier (non-split) diff and the reason for the 
`while` is that there are a lot of conditional moves, advancements and even an 
`erase` call in the loop body.


https://reviews.llvm.org/D52795



___
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 planned changes to this revision.
whisperity added a comment.

In https://reviews.llvm.org/D53334#1288057, @dblaikie wrote:

> In https://reviews.llvm.org/D53334#1273877, @whisperity wrote:
>
> > @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.
>
>
> Hey, sorry for the delay - you can use "clang -### " (or 
> "clang++ -### " to get clang to print out the underlying 
> -cc1 command line that is used.
>
> If you're changing the driver then we'd write a driver test (that tests that, 
> given "clang -foo -###" it produces some "clang -cc1 -bar" command line to 
> run the frontend.
>
> But since you're changing the driver, it's not interesting to (re) test how 
> --precompile is lowered from the driver to cc1. Instead we test the frontend 
> (cc1) directly.


Understood, thank you for the help, I'll try looking into this. It was just at 
first a strange behaviour to see that conventionally an external call command 
messes up and behaves different.
Until then I'll re-mark this as a WIP because the tests are half-baked anyways.


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 172908.
whisperity added a comment.

Test was added.


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,30 @@
+// 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 -DHAS_PTHREAD=1 \
+// RUN: %S/Inputs/module-mismatch.cppm  \
+// RUN: -o %t/prebuilt_modules/module_mismatch.pcm
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple  \
+// RUN: -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -emit-module-interface  \
+// RUN: %S/Inputs/module-mismatch.cppm  \
+// RUN: -o %t/prebuilt_modules/module_no_mismatch.pcm
+//
+// RUN: not %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DMISMATCH_CHECK \
+// RUN: %s 2>&1 | FileCheck %s 
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules %s
+
+#ifdef MISMATCH_CHECK
+import module_mismatch;
+// CHECK: error: POSIX thread support was enabled in PCH file but is currently 
disabled
+// CHECK-NEXT: module file {{.*}}/module_mismatch.pcm cannot be loaded due to 
a configuration mismatch with the current compilation
+#else
+import module_no_mismatch; // expect-no-diagnostics
+#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,30 @@
+// 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 -DHAS_PTHREAD=1 \
+// RUN: %S/Inputs/module-mismatch.cppm  \
+// RUN: -o %t/prebuilt_modules/module_mismatch.pcm
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple  \
+// RUN: -fmodules-ts -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -emit-module-interface  \
+// RUN: %S/Inputs/module-mismatch.cppm  \
+// RUN: -o %t/prebuilt_modules/module_no_mismatch.pcm
+//
+// RUN: not %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DMISMATCH_CHECK \
+// RUN: %s 2>&1 | FileCheck %s 
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fmodules-ts \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules %s
+
+#ifdef MISMATCH_CHECK
+import module_mismatch;
+// CHECK: error: POSIX thread support was enabled in PCH file but is currently disabled
+// CHECK-NEXT: module file {{.*}}/module_mismatch.pcm cannot be loaded due to a configuration mismatch with the current compilation
+#else
+import module_no_mismatch; // expect-no-diagnostics
+#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_mi

[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] D54757: [clang-tidy] new check: bugprone-branch-clone

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Bar the previous comments, I really like this. The test suite is massive and 
well-constructed. Do we know of any real-world findings, maybe even from LLVM?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54757



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In https://reviews.llvm.org/D54757#1305306, @Eugene.Zelenko wrote:

> In https://reviews.llvm.org/D54757#1305114, @whisperity wrote:
>
> > Bar the previous comments, I really like this. The test suite is massive 
> > and well-constructed. Do we know of any real-world findings, maybe even 
> > from LLVM?
>
>
> GCC 7 introduced -Wduplicated-branches, so will be good idea to run this 
> check on associated regression(s).


Compared to Donát's checker, GCC's warning seems much more underperforming. See 
https://godbolt.org/z/Iq3FC9.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54757



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

[PATCH] D57855: [analyzer] Reimplement checker options

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:49
+  "NoDiagnoseCallsToSystemHeaders",
+  "",
+  "false">

Let's put at least a FIXME here that the documentation for this option was 
missing.



Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:907
+  "TrackNSCFStartParam",
+  "",
+  "false">

Same here for missing documentation/description for optstring.



Comment at: lib/Frontend/CompilerInvocation.cpp:425
   bool HasFailed = getStringOption(Config, Name, std::to_string(DefaultVal))
- .getAsInteger(10, OptionField);
+ .getAsInteger(0, OptionField);
   if (Diags && HasFailed)

What is this trying to do?


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

https://reviews.llvm.org/D57855



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

[PATCH] D58065: [analyzer] Document the frontend library

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: docs/analyzer/developer-docs/FrontendLibrary.rst:85
+-fuse-ld=lld \
+../llvm
+

george.karpenkov wrote:
> Information on compiling LLVM IMO should not be here.
> Also, why Sphinx? Why X86? Why LLD and not gold?
> Information on compiling LLVM IMO should not be here.
> Also, why Sphinx? Why X86? Why LLD and not gold?

Analyser for development -- however, you build `release` instead of (at least) 
`RelWithDebInfo`.

`EXPORT_COMPILE_COMMANDS` is also totally unnecessary for an "analyser for 
development".

Perhaps individual recommended settings could be linked back to the 
configuration guide's sections? (Unfortunately that guide can't link to 
individual //options// sadly.)



Comment at: docs/analyzer/developer-docs/FrontendLibrary.rst:94
+Other documents detail the difference between the *driver* and the *frontend*
+of clang far more precisely, but we'll touch on this briefly: When you input
+``clang`` into the command line, you invoke the driver. This compiler driver

When we talk about the project, use **C**lang. When talking about the binary, 
`clang` in monospace is good style.



Comment at: docs/analyzer/developer-docs/FrontendLibrary.rst:140
+
+Although we don't support running the analyzer without enabling the entire core
+package, it is possible, but might lead to crashes and incorrect reports.

`core` for emphasis.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58065



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

[PATCH] D57860: [analyzer] Validate checker option names and values

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:329
+
+  // Insertation was successful -- CmdLineOption's constructor will validate
+  // whether values received from plugins or TableGen files are correct.

Insertion



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:340
+  if (OptionType == "bool") {
+if (SuppliedValue != "true" && SuppliedValue != "false") {
+  if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {

Does this mean that we no longer can give "1" and "0" for boolean checker 
options? Or was that //Tidy// who allowed such in the first place?


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

https://reviews.llvm.org/D57860



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

[PATCH] D58573: [analyzer] Move UninitializedObject out of alpha

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In case no bug reports //against// the checker are reported on the mailing list 
or Bugzilla, I wholeheartedly agree with kicking the ball here.

As for the package, perhaps we could go into `optin.bugprone` or 
`optin.conventions`? (These are new package names...)


Repository:
  rC Clang

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

https://reviews.llvm.org/D58573



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

[PATCH] D57860: [analyzer] Validate checker option names and values

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:325
+  std::string FullOption =
+  (llvm::Twine() + FullName + ":" + OptionName).str();
+

baloghadamsoftware wrote:
> Is this the most efficient way to concatenate `StringRef`s?
> @baloghadamsoftware 
> Is this the most efficient way to concatenate `StringRef`s?

`Twine` is specifically designed for that, yes.
You can't evade the final "copy", although most likely there'll be a RVO taking 
place.

However, the *first* `Twine` could be initialised from the `FullName` variable, 
as opposed to empty node.
And you could use single characters as characters, as opposed to strings,  i.e. 
`(Twine(FullName) + ':' + OptionName).str()`.


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

https://reviews.llvm.org/D57860



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

[PATCH] D59465: [analyzer] Add example plugin for checker option handling

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@Szelethus I know the dependent patch D59464  
will move `examples/analyzer-plugin` to `test/Analysis/plugins/...`, but this 
patch still seems to affect `examples/`. Are you sure this is the right diff? 
Because you are adding brand new files and brand new "folders", I don't think 
that'll apply cleanly.




Comment at: test/Analysis/checker-plugins.c:35
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -load %llvmshlibdir/OptionHandlingAnalyzerPlugin%pluginext\
+// RUN:   -analyzer-checker=example.MyChecker \

Why isn't here a space before the line break escape `\`, but in the other lines?


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

https://reviews.llvm.org/D59465



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

[PATCH] D57922: [analyzer] Insert checker options into AnalyzerOption::ConfigTable

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

I think this is good. Patch still marked as //Needs review// for some reason. 😦 
Can we look up this `blocking review` thing? Perhaps this could be marked ready 
to roll once the dependency patch is ironed out.




Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:332
+  AnOpts.Config.insert({(Twine() + CheckerFullName + ":" + OptionName).str(),
+DefaultValStr});
 }

baloghadamsoftware wrote:
> `Twine(CheckerFullName) + ":" + OptionName`. However, since the constructor 
> `Twine(const StringRef &Str)` is implicit, `CheckerFullName + ":" + 
> OptionName` results the same and is more readable.
This comment is **Done**.


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

https://reviews.llvm.org/D57922



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

[PATCH] D57858: [analyzer] Add a new frontend flag to display all checker options

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@dcoughlin How would removing the `USAGE` part of the dump and keeping only the 
list of options and their formatted help sound? That way, this option will not 
invite the user to directly call the analyzer.

In D57858#1432714 , @dcoughlin wrote:

> - The help text also recommends invoking -cc1 directly or through the driver 
> with -Xclang. Neither of these are supported end-user interfaces to the 
> analyzer.


Calling this option itself, at least based on the original commit's first line, 
is through `-cc1`, and thus using a "checker/SA developer interface". This 
seems more purely as a tooling help, which as expressed by @dkrupp earlier, is 
helpful for "wrangler tools".

@Szelethus From a CodeChecker guy's perspective, I am a bit scared about the 
size this dump can get assuming every option is given a description/help text 
nicely, but all in all I like the direction of this patch.




Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:584-592
+  Out << "USAGE: clang -cc1 [CLANG_OPTIONS] -analyzer-config "
+
"\n\n";
+  Out << "   clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE, "
+  "-analyzer-config OPTION2=VALUE, 
...\n\n";
+  Out << "   clang [CLANG_OPTIONS] -Xclang -analyzer-config -Xclang"
+
"\n\n";
+  Out << "   clang [CLANG_OPTIONS] -Xclang -analyzer-config -Xclang "

(I mean killing these lines.)


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

https://reviews.llvm.org/D57858



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

[PATCH] D61260: [clang-tidy] Extend bugprone-sizeof-expression to check sizeof(pointers to structures)

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: test/clang-tidy/bugprone-sizeof-expression.cpp:196
   typedef const MyStruct TMyStruct;
+  typedef const MyStruct *PMyStruct;
 

While I trust Clang and the matchers to unroll the type and still match, I'd 
prefer also adding a test case for

```
typedef TMyStruct* PMyStruct2;
```

or somesuch.

And perhaps a "copy" of these cases where they come from template arguments, in 
case the checker can also warn for that?



Comment at: test/clang-tidy/bugprone-sizeof-expression.cpp:231
+  sum += sizeof(MyStruct*);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);

Why is this printed at `sizeof(A*)`? Do we not print the name of the actual 
type used in the expression?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61260



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://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.

I have checked the results, thank you for uploading them, they look solid to 
me, although I'm not exactly a developer for these projects, without full 
understanding of what and where allocates and true path-sensitive analysis and 
memory modelling, they look good. (E.g. one thing this check misses I think is 
when the allocator returns an explicitly zero-filled memory, because that way 
the write without the good size is //still// NUL-terminated... but this 
requires modelling we might just not be capable of, especially not in 
Clang-Tidy.)

With a bit of focused glancing, the check's code is also understandable, 
thanks. :)


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] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like elements

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Hey @mgrang! Let's see some results on some projects and answer NoQ's comment, 
then I think we can put this in for all the world to see.


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] D52390: [analyzer] Add StackSizeChecker to StaticAnalyzer

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

One final nit apart from a few in-line comments (most of them are just arguing 
with @Szelethus anyways 😛): KB vs. KiB  
(remember how a 2 TB hard drive appears as 1.8 TB on your machine? Because it's 
TiB!) is one of my pet peeves especially that Windows and most people //still// 
get it wrong nowadays (and they also teach it wrong). Can we change the default 
from `100 000` to `102 400`? That would make it truly `100 KiB` in the proper 
sense.




Comment at: 
include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:16
+//===--===//
+
+#include "llvm/ADT/DenseMap.h"

Szelethus wrote:
> Missing header guard.
Speaking of missing header guards and in general: apart from tests, it might be 
a good idea to run CSA and Clang-Tidy //on your checker//'s code. E.g. Tidy has 
a [[ https://clang.llvm.org/extra/clang-tidy/checks/llvm-header-guard.html | 
`llvm-header-guard` ]] checker -- which might //not// find this particular 
issue, but in general we could find some issues on this code itself.

(For the ultimate version, run your own checker on your own checker's code... 
maybe you yourself used a too large stack somewhere! 😜 )



Comment at: 
include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:102-111
+  bool hasEmptyFlaggedUsageStored(const Stmt *S) const {
+auto iter = StmtSubtreeUsages.find(S);
+return iter != StmtSubtreeUsages.end() &&
+   iter->second == Usage::EmptyFlagged;
+  }
+  bool hasEmptyFlaggedUsageStored(const Decl *D) const {
+auto iter = DeclSubtreeUsages.find(D);

Szelethus wrote:
> Would `isEmptyFlagged` be a better method name?
Then calling this method would mean answering this question: "A Stmt/Decl is 
empty flagged?" This is a "What?" moment.



Comment at: 
include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:117
+
+  int varSize(const VarDecl *Var) const {
+return Var->hasLocalStorage() * sizeInBytes(Var->getType());

If we're talking about sizes, `int` (especially because it's //`signed`// 
`int`) might not be the most appropriate return type for these functions.



Comment at: 
include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:124
+   ? 0
+   : Context.getTypeSize(T) / Context.getCharWidth();
+  }

Szelethus wrote:
> The function name states `sizeInBytes`, but
> * `ASTContext::getTypeSizeInChars` returns the width in `CharUnits`,
> * which you divide with `Context.getCharWidth()`, which is in bits (according 
> to the doxygen comments),
> * and we end up getting the amount of bytes `T` occupies?
> I might make a fool out of myself, but wouldn't the formula look something 
> like this: `(Context.getTypeSize(T) * Context.getCharWidth()) / 8`?
Need to make a distinction whether or not we want to use `byte` in the sense of 
`8 bit` or in the sense of `sizeof(char)`. Also, the code calls `getTypeSize`, 
not `getTypeSize`~~`InChars`~~. This gives the answer in pure bits.

(Because of this, your suggested formula is wrong. `getTypeSize()` is already 
in bits, which you multiply further by `getCharWidth()` -- which is //on some 
systems// 8 -- then you divide it out. In most conventional systems the 
division cancels out the multiplication, and on systems where a 
`sizeof(char)`-byte is not 8, you'll get a non-integer quotient truncated as 
result.) 

So:

 * in case of you want to define `byte` to be `8 bits` forever and always, the 
divisor should be `8`, not `getCharWidth()`.
 * In case you want to define `byte` to be `sizeof(char)`, then the current 
code **is** correct, but you could just use: [[ 
https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#aa8c44e88d6ce9b3cce885b62d3cd5dff
 | `Context.getTypeSizeInChar()` ]] `.` [[ 
https://clang.llvm.org/doxygen/classclang_1_1CharUnits.html#a968b1a66a449ab256c4dd176bd663c07
 | `getQuantity()` ]].

Just for the record: the C++14 standard says the following in § 1.7 
`intro.memory`:

> The fundamental storage unit in the C++ memory is the **byte**.  A byte is 
> //at least large enough// to contain any member of the basic execution 
> character set (-> § 2.3 `lex.charset`) and the eight-bit code units of the 
> Unicode UTF-8 encoding form and is composed of a contiguous sequence of bits, 
> **the number of which is implementation-defined**.

(`lex.charset` is basically an ASCII subset of 96 cardinality, lowercase, 
uppercase, parens, etc.)

I'm not sure what the convention is for error reports when referring to bytes, 
but maybe it would be better to consider `byte` in the C++ sense and let the 
value change based on what target the analyzer file is being compiled for.



Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:25
+#include "clang/StaticAna

[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 subscriber: baloghadamsoftware.
whisperity added inline comments.



Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:1038
+
+  SmallString<128> NewAddNullTermExprStr;
+  NewAddNullTermExprStr = "\n";

aaron.ballman wrote:
> This should be done using a `Twine`.
Can you please give an example of how to well-approach this? We had issues with 
using Twines on the stack and then later on running into weird undefined 
behaviour. The documentation is not clear to a lot of our colleagues on where 
the `Twine`'s usage barriers are... (Asking this not just for @Charusso but 
also for a few more colleagues, @baloghadamsoftware e.g.)


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] D52742: [analyzer][WIP] Add macro expansions to the plist output

[Whisperity via Phabricator via cfe-commits]

whisperity added a subscriber: gamesh411.
whisperity added a comment.

Your code looks good, just minor comments going on inline.
Trying to think of more cases to test for, in case someone generously misuses 
FLMs, as seen here in an example if you scroll down a bit: 
https://gcc.gnu.org/onlinedocs/cpp/Macro-Arguments.html#Macro-Arguments
Maybe one or two tests cases for this should be added, but I believe all 
corners are covered other than this.

(The whole thing, however, is generally disgusting. I'd've expected the 
`Preprocessor` to readily contain //a lot of stuff// that's going on here.)




Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:689
 
+  /// Returns with true if macros related to the bugpath should be expanded and
+  /// included in the plist output.

Returns ~~with ~~ true



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:261
+  std::string Expansion;
+  ExpansionInfo(std::string N, std::string E) : MacroName(N), Expansion(E) {}
+};

move move move like in your other self-defined type below



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:292
+
+  // Output the location.
+  FullSourceLoc L = P.getLocation().asLocation();

the location of what?



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:298
+
+  // Output the ranges (if any).
+  ArrayRef Ranges = P.getRanges();

the ranges of what?



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:668
+/// need to expanded further when it is nested inside another macro.
+class MacroArgsInfo : public std::map {
+public:

You have two records named "MacroArgsInfo" and "MacroNameAndArgsInfo", this is 
confusing. A different name should be for this class here... maybe the later 
used `MacroArgMap` is good, and then the struct's field should be renamed 
`ArgMap` or just `Args`.



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:680
+   llvm::Optional M)
+: Name(N), MI(MI), MacroArgMap(M) {}
+};

move string argument into local member (See 
https://www.bfilipek.com/2018/08/init-string-member.html, it's fascinating and 
me and @gamesh411  just found this again yesterday night.)



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:711-712
+static ExpansionInfo getExpandedMacroImpl(SourceLocation MacroLoc,
+  const Preprocessor &PP,
+  MacroArgsInfo *PrevArgMap) {
+

Indentation of these lines is wrong.



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:723
+
+  // If this macro function-like.
+  if (MacroArgMap) {

"this is a" or "macro is function-like"



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:725
+  if (MacroArgMap) {
+// If this macro is nested inside another one, let's manually expand it's
+// arguments from the "super" macro.

its



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:726
+// If this macro is nested inside another one, let's manually expand it's
+// arguments from the "super" macro.
+if (PrevArgMap)

What's a "\"super\" macro"? Maybe "outer" (or "outermost") is the word you 
wanted to use here?



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:737
+
+  for (auto It = MI->tokens_begin(), E = MI->tokens_end(); It != E;) {
+Token T = *It;

Unnecessary `;` at the end?



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:756
+  while ((++It)->isNot(tok::r_paren)) {
+assert(It->isNot(tok::eof));
+  }

`&& "Ill-formed input: macro args were never closed!"`



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:765
+  if (MacroArgMap && MacroArgMap->count(II)) {
+for (Token ExpandedArgT : MacroArgMap->at(II)) {
+  ExpansionOS << PP.getSpelling(ExpandedArgT) + ' ';

`Token&` so copies are explicitly not created?



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:796
+  std::pair LocInfo = SM.getDecomposedLoc(ExpanLoc);
+  const char *MacroNameBuf = LocInfo.second + BufferInfo.data();
+

Okay, this looks nasty. I get that pointer offsetting is commutative, but... 
this is nasty.

Also, what's the difference between using `.data()` and `.begin()`? `Lexer`'s 
this overload takes three `const char*` params. Maybe this extra variable here 
is useless and you could just pass `BufferInfo.data() + LocInfo.second` to the 
constructor below.



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:796
+  std::pair LocInfo = SM.getDecomposedLoc(ExpanLoc);
+  const char *MacroNameBuf = LocInfo.secon

[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] 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] D30691: [analyzer] Support for naive cross translational unit analysis

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

The Python code here still uses `mangled name` in their wording. Does this mean 
this patch is yet to be updated with the USR management in the parent patch?




Comment at: tools/scan-build-py/libscanbuild/analyze.py:165
+with open(filename, 'r') as in_file:
+for line in in_file:
+yield line

danielmarjamaki wrote:
> I believe you can write:
> 
> for line in open(filename, 'r'):
Do we want to rely on the interpreter implementation on when the file is closed.

If 

```
  for line in open(filename, 'r'):
 something()
```

is used, the file handle will be closed based on garbage collection rules. 
Having this handle disposed after the iteration is true for the stock CPython 
implementation, but it is still nontheless an implementation specific approach.

Whereas using `with` will explicitly close the file handle on the spot, no 
matter what.



Comment at: tools/scan-build-py/libscanbuild/analyze.py:172
+extern_fns_map_file = os.path.join(ctudir, CTU_FUNCTION_MAP_FILENAME)
+with open(extern_fns_map_file, 'w') as out_file:
+for mangled_name, ast_file in mangled_ast_pairs:

danielmarjamaki wrote:
> this 'with' seems redundant. I suggest an assignment and then less 
> indentation will be needed below
I don't seem to understand what do you want to assign to what.


https://reviews.llvm.org/D30691



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

[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:195-196
+  llvm::raw_svector_ostream OS(SBuf);
+  OS << "Null pointer passed as an argument to a 'nonnull' ";
+  OS << Idx << llvm::getOrdinalSuffix(Idx) << " parameter";
+

It seems to be as if now you're able to present which parameter is the 
`[[nonnull]]` one. Because of this, the output to the user sounds really bad 
and unfriendly, at least to me.

How about this:

"null pointer passed to nth parameter, but it's marked 'nonnull'"?
"null pointer passed to nth parameter expecting 'nonnull'"?

Something along these lines.

To a parameter, we're always passing arguments, so saying "as an argument" 
seems redundant.

And because we have the index always in our hands, we don't need to use the 
indefinite article.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66333



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

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@NoQ Do you reckon these tests files are too long? Perhaps the one about this 
inheritance, that inheritance, diamond inheritance, etc. could be split into 
multiple files.




Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:317
 
+def CtorUninitializedMemberChecker: Checker<"CtorUninitializedMember">,
+ HelpText<"Reports uninitialized members in constructors">,

This always pokes me in the eye, but shouldn't this file be sorted 
alphabetically?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:103
+  // - pointer/reference
+  // - fundamental object (int, double, etc.)
+  //   * the parent of each node is the object that contains it

I believe `fundamental object` should be rephrased as `of fundamental type` (as 
in: object that is of fundamental type), as the standard talks about 
//fundamental types//.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:187
+
+  if (isCalledByConstructor(Context))
+return;

I think somewhere there should be a bit of reasoning why exactly these cases 
are ignored by the checker.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:218
+
+  std::string WarningMsg = std::to_string(UninitFields.size()) +
+   " uninitialized field" +

Maybe one could use a Twine or a string builder to avoid all these unneccessary 
string instantiations? Or `std::string::append()`?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:316
+
+// At this point the field is a fundamental type.
+if (isFundamentalUninit(V)) {

(See, you use //fundamental type// here.)



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:340
+
+// TODO As of writing this checker, there is very little support for unions in
+// the CSA. This function relies on a nonexisting implementation by assuming as

Please put `:` after `TODO`.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:342
+// the CSA. This function relies on a nonexisting implementation by assuming as
+// little about it as possible.
+bool FindUninitializedFields::isUnionUninit(const TypedValueRegion *R) {

What does `it` refer to in this sentence?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:421
+if (T->isUnionType()) {
+  // TODO does control ever reach here?
+  if (isUnionUninit(RT)) {

Please insert a `:` after `TODO` here too.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:475
+  if (Chain.back()->getDecl()->getType()->isPointerType())
+return OS.str().drop_back(2);
+  return OS.str().drop_back(1);

Maybe this could be made more explicit (as opposed to a comment) by using 
[[http://llvm.org/doxygen/classllvm_1_1StringRef.html#a3f45a78650a72626cdf9210de0c6d701|`StringRef::rtrim(StringRef)`]],
 like this:

return OS.str().rtrim('.').rtrim("->");

How does this code behave if the `Chain` is empty and thus `OS` contains no 
string whatsoever? `drop_back` asserts if you want to drop more elements than 
the length of the string.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:498
+Context.getStackFrame());
+  // getting 'this'
+  SVal This = Context.getState()->getSVal(ThisLoc);

Comment isn't formatted as full sentence.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:501
+
+  // getting '*this'
+  SVal Object = Context.getState()->getSVal(This.castAs());

Neither here.



Comment at: test/Analysis/ctor-uninitialized-member.cpp:683
+// Note that the rules for unions are different in C++ and C.
+// http://lists.llvm.org/pipermail/cfe-dev/242017-March/42052912.html
+

NoQ wrote:
> I managed to find the thread, but this link doesn't work for me.
Confirmed, this link is a 404.


https://reviews.llvm.org/D45532



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

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Whisperity via Phabricator via cfe-commits]

whisperity requested changes to this revision.
whisperity added subscribers: gsd, dkrupp, o.gyorgy.
whisperity added a comment.
This revision now requires changes to proceed.

Sorry, one comment has gone missing meanwhile, I'm still getting used to this 
interface and hit //Submit// early.




Comment at: test/Analysis/ctor-uninitialized-member-inheritance.cpp:24
+  : NonPolymorphicLeft1(int{}) {
+y = 420;
+z = 420;

The literal `420` is repeated //everywhere// in this file. I think this (the 
same value appearing over and over again) will make debugging bad if something 
goes haywire and one has to look at memory dumps, control-flow-graphs, etc.


https://reviews.llvm.org/D45532



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

[PATCH] D45407: [StaticAnalyzer] Added notes to the plist output

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@NoQ The problem with emitting notes as events is that we lose the information 
that the node was a `node`. How does Xcode behave with these notes? Does it 
ignore them, or can read them from the command-line output of the analyser?


Repository:
  rC Clang

https://reviews.llvm.org/D45407



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

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In https://reviews.llvm.org/D45532#1068700, @Szelethus wrote:

> In https://reviews.llvm.org/D45532#1068647, @dkrupp wrote:
>
> > This bug report also mentions assignment operator. But for that a warning 
> > may be not so useful. In that case the members of the assigned to object 
> > should have some initialized value already which the programmer may not 
> > want to overwrite in the assignment operator.
>
>
> I believe there's a checker for that already, but I'm really not sure whether 
> `UndefinedAssignmentChecker` covers all such cases.


Indeed, there are two different targets for analysis here:

- The `rhs` of the assignment contains an uninitialised value, that is 
copied/moved into the `this` of the assignment. --> This //should// be checked 
by `UndefinedAssignmentChecker`.
- Not every field is initialised by the assignment operator. This one, I 
believe, is hard to check and prove, and also feels superfluous. As @NoQ wrote 
earlier, the amount of reports this checker emits in itself is, at least 
expected to be, huge, and while it can be a valid programming approach that a 
ctor must initialise all, tracking in the CSA on an `operator=` that the 
`this`-side had something uninitialised but it was also not initialised by the 
assignment? Hard, and also not very useful, as far as I see.



In https://reviews.llvm.org/D45532#1064652, @NoQ wrote:

> That said, your checker finds non-bugs, which is always risky. Sometimes the 
> user should be allowed to initialize the field after construction but before 
> the first use as a potentially important performance optimization. Did you 
> find any such intentionally uninitialized fields with your checker? Were 
> there many of those?


Where I can vision the usefulness of this checker **the most** is code that is 
evolving. There are many communities and codebases where coding standards and 
practices aren't laid out in a well-formed manner like LLVM has. There are also 
projects which are traditionally C code that has evolved into C++-ish code. 
When moving into C++, people start to realise they can write things like 
constructors, so they write them, but then leave out some members, and we can 
only guess (and **perhaps** make use of other checkers related to dereference 
or read of undefineds) what needs to be initialised, what is used without 
initialisation.

This checker is more close to something like a `bugprone-` Tidy matcher from 
the user's perspective, but proper analysis requires it to be executed in the 
SA.

A valid constriction of what this checker can find could be members that can 
"seemingly" be only be set in the constructor, there are no write operations or 
public exposure on them.

I have reviewed some findings of the checker, consider the following very 
trivial example:

  #define HEAD_ELEMENT_SPECIFIER 0xDEADF00D // some magic yadda
  
  struct linked_list;
  
  struct linked_list
  {
  int elem;
  linked_list* next;
  };
  
  class some_information_chain_class
  {
public:
  some_information_chain_class() : iList(new linked_list())
  {
iList->elem = HEAD_ELEMENT_SPECIFIER;
  }
  
private:
  linked_list* iList;
  };

In this case, the "next" is in many cases remain as a garbage value. Of course, 
the checker cannot know, if, for example, there is also a `count` field and we 
don't rely on `next == nullptr` to signify the end of the list. But this can 
very well be a mistake from the programmer's end.

> If a user wants to have his codebase "analyzer-clean", would he be able to 
> suppress the warning without changing the behavior of the program?

I'm not entirely sure what you mean by "changing the behaviour of the program". 
As far as I can see, this checker only reads information from the SA, so it 
should not mess up any preconception set or state potentially used by other 
checkers.

---

In https://reviews.llvm.org/D45532#1065716, @Szelethus wrote:

> I didn't implement anything explicitly to suppress warnings, so if there is 
> nothing in the CSA by default for it, I'm afraid this isn't possible (just 
> yet).


So far the only thing I can think of is coming up with new command-line 
arguments that can fine-tune the behaviour of the checker - but in this case, 
you can just as well just toggle the whole thing to be disabled.

An improvement heuristic (implemented in a later patch, toggleable with a 
command-line option perhaps?), as discussed above, would be checking //only// 
for members for whom there isn't any "setter" capability anywhere in the type.


https://reviews.llvm.org/D45532



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

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Whisperity via Phabricator via cfe-commits]

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

There is something that came up in my mind:

Consider a construct like this:

  class A
  {
A()
{
  memset(X, 0, 10 * sizeof(int));
}
  
int X[10];
  };

I think it's worthy for a test case if this `X` is considered unitialised at 
the end of the constructor or not. (And if not, a ticket, or a fix for SA in a 
subpatch.)


https://reviews.llvm.org/D45532



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

[PATCH] D45095: [clang-tidy] Align usage of ClangTool interface with new VFS injection

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tidy/tool/CMakeLists.txt:19
   clangBasic
+  clangFrontend
   clangTidy

ilya-biryukov wrote:
> whisperity wrote:
> > ilya-biryukov wrote:
> > > Why do we need an extra dependency?
> > In the current state of the patch, `clang-tidy/tool/ClangTidyMain.cpp` 
> > constructs the `ClangTool` which constructor requires a 
> > `std::shared_ptr`. `PCHContainerOperations`'s 
> > definition and implementation is part of the `clangFrontend` library, and 
> > without this dependency, there would be a symbol resolution error.
> Thanks for clarification.
> Does it mean that to use `ClangTool` one needs both a dependency on 
> `clangTooling` and `clangFrontend`?
> That's weird, given that `clangTooling` itself depends on `clangFrontend` and 
> it would be nice if the buildsystem actually propagated those.
I'm not sure if you need to have both as a dependency just to use `ClangTool`. 
If you want to construct an empty `PCHContainerOperations` to pass as an 
argument, though, it seems you do.

One can wonder where the default argument's (which is a shared pointer to a 
default constructed object) expression's evaluation is in the object code or if 
passing `nullptr` breaks something.

You need the dependency because it's **your** code who wants to run the 
constructor of the PCH class.


https://reviews.llvm.org/D45095



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

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@george.karpenkov @NoQ `bugprone.` as a category sounds nice. It also nicely 
corresponds to the Clang-Tidy `bugprone-` category. It would not be nice to 
further fragment the "top levels" of checker categories.

I can say with confidence that CodeChecker does not break if the same category 
name is used by two different analyzers. Does the same stand for XCode / 
Scan-Build? In this case, introducing the `bugprone` category with the same 
principle that is behind Tidy's one is a good step.




Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:162
+/// We can't know the type of the region that a void pointer points to, so FD
+/// can't be analyzed if this function return true for it.
+bool isVoidPointer(const FieldDecl *FD);

george.karpenkov wrote:
> "returns"
Actually, this explanation is superfluous. I believe

Returns if FD can be (transitively) dereferenced to a void* (void**, ...). 
The type of the region behind a void pointer isn't known, and thus FD can not 
be analyzed.

should suffice?



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:212
+
+  std::string WarningMsg = std::to_string(UninitFields.size()) +
+   " uninitialized field" +

george.karpenkov wrote:
> nitpicking: llvm::Twine is normally used for such constructs
I have also suggested the usage of Twine for this line (it's just the diff that 
got out of sync with the line numbers!), but I don't recall what @Szelethus' 
concern was about them. In case we have move semantics enabled, this line will 
compile into using the moving concatenator.


https://reviews.llvm.org/D45532



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://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 a comment.

While I understand extending the analyzer to cover more is a good approach, 
there is `-Wconversion` which seemingly covers this -- or at least the trivial 
case(?):

  #include 
  #include 
  
  void foo(unsigned x)
  {
printf("%u\n", x);
  }
  
  int main()
  {
int i = -1;
foo(i);
  
std::vector x;
x[i] = 5;
  }



  $ clang++ -Wconversion -o/dev/null check.cpp 
  check.cpp:12:7: warning: implicit conversion changes signedness: 'int' to 
'unsigned int' [-Wsign-conversion]
foo(i);
~~~ ^
  check.cpp:15:5: warning: implicit conversion changes signedness: 'int' to 
'std::vector::size_type' (aka 'unsigned long') [-Wsign-conversion]
x[i] = 5;
~ ^


Repository:
  rC Clang

https://reviews.llvm.org/D46081



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

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Two minor comments.

With regards to the function naming, the problem with incremental counts is 
that they get out of sync, like they did with `fXpY` and such, and if someone 
wants to keep the count incremental down the file, adding any new test will 
result in an unnecessarily large patch. Perhaps you could use `void T_()` for 
the test that calls `T::T()`?




Comment at: test/Analysis/cxx-uninitialized-object.cpp:660
+}
+#endif // PEDANTIC
+class MultiPointerTest3 {

A newline between `#endif` and the next token is missing here.



Comment at: test/Analysis/cxx-uninitialized-object.cpp:1412
+  struct RecordType;
+  // no-crash
+  RecordType *recptr; // expected-note{{uninitialized pointer 'this->recptr}}

What is this comment symbolising? Is this actually something the test 
infrastructure picks up?


https://reviews.llvm.org/D45532



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

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Pinging this as the talk has stalled.


https://reviews.llvm.org/D45094



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

[PATCH] D32747: [Analyzer] Iterator Checker - Part 3: Invalidation check, first for (copy) assignments

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:92
+  // Whether iterator is valid
+  bool Valid;
+

Seeing that in line 106 you consider this record immutable, you might want to 
add a `const` on this field too.



Comment at: test/Analysis/invalidated-iterator.cpp:32
+  *i0; // expected-warning{{Invalidated iterator accessed}}
+}

This might not be applicable here, but isn't there a test case for copy 
operations where the iterator isn't invalidated? Something of a positive test 
to be added here. My only idea is something with weird reference-to-pointer 
magic. Might not worth the hassle.


https://reviews.llvm.org/D32747



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

[PATCH] D32845: [Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function parameters

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:317
 
+def MismatchedIteratorChecker : Checker<"MismatchedIterator">,
+  HelpText<"Check for use of iterators of different containers where iterators 
of the same container are expected">,

Is there any particular order entries of this file should be in? Seems to be 
broken now, but I guess when this patch comes up to the top of the stack it 
shall be fixed at a rebase.



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:426
+  // For every template parameter which is an iterator type in the
+  // instantiation look for all functions parameters type by it and
+  // check whether they belong to the same container

functions' parameters' ?



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:934
+}
+reportMismatchedBug("Iterator access mismatched.", Iter1, Iter2, C, N);
+  }

If this string is the message that gets printed to the user, I think it must be 
rephrased a bit. If this message came across me, I'd be puzzled at first to 
understand what it is trying to mean.


https://reviews.llvm.org/D32845



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

[PATCH] D32859: [Analyzer] Iterator Checker - Part 5: Move Assignment of Containers

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1061
+  // first reassign all iterator positions to the new container which
+  // are not past the container (thus not greater or equal to the
+  // current "end" symbol.

`(` is not closed


https://reviews.llvm.org/D32859



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

[PATCH] D69560: [clang-tidy] Add 'cppcoreguidelines-avoid-adjacent-arguments-of-same-type' check

[Whisperity via Phabricator via cfe-commits]

whisperity created this revision.
whisperity added reviewers: aaron.ballman, alexfh, Eugene.Zelenko, JonasToth, 
NoQ, Szelethus, xazax.hun, baloghadamsoftware, Charusso.
whisperity added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, ormris, kbarton, mgorny, nemanjai.
Herald added a project: clang.

This check finds function definitions where arguments of the same type follow 
each other directly, making call sites prone to calling the function with 
swapped or badly ordered arguments.

The relevant C++ Core Guidelines rules to which conformity is checked is found 
at: 
http://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#i24-avoid-adjacent-unrelated-parameters-of-the-same-type


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69560

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-adjacent-arguments-of-same-type.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-cvr-on.cpp
  
clang-tools-extra/test/clang-tidy/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-default.cpp
  
clang-tools-extra/test/clang-tidy/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-verbose.cpp

Index: clang-tools-extra/test/clang-tidy/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-verbose.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-verbose.cpp
@@ -0,0 +1,420 @@
+// RUN: %check_clang_tidy %s \
+// RUN:   cppcoreguidelines-avoid-adjacent-arguments-of-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.MinimumLength, value: 2}, \
+// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.CVRMixPossible, value: 1} \
+// RUN:   ]}' --
+
+void library(void *vp, void *vp2, void *vp3, int n, int m);
+// NO-WARN: The user has no chance to change only declared (usually library)
+// functions, so no diagnostic is made.
+
+struct T {};
+
+void create(T **out_t) {} // NO-WARN
+
+void copy(T *p, T *q, int n) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 2 adjacent arguments for 'copy' of similar type ('T *') are easily swapped by mistake [cppcoreguidelines-avoid-adjacent-arguments-of-same-type]
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: last argument in the adjacent range here
+
+void mov(T *dst, const T *src) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 2 adjacent arguments for 'mov' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:27: note: last argument in the adjacent range here
+// CHECK-MESSAGES: :[[@LINE-3]]:18: note: at a call site, 'const T *' might bind with same force as 'T *'
+
+void compare01(T t1, T t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'compare01' of similar type ('T') are
+// CHECK-MESSAGES: :[[@LINE-2]]:24: note: last argument in the adjacent range
+
+void compare02(const T t1, T t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'compare02' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:30: note: last argument in the adjacent range
+// CHECK-MESSAGES: :[[@LINE-3]]:28: note: at a call site, 'T' might bind with same force as 'const T'
+
+void compare03(T t1, const T t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'compare03' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:30: note: last argument in the adjacent range
+// CHECK-MESSAGES: :[[@LINE-3]]:22: note: at a call site, 'const T' might bind with same force as 'T'
+
+void compare04(const T &t1, T t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'compare04' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:31: note: last argument in the adjacent range
+// CHECK-MESSAGES: :[[@LINE-3]]:29: note: at a call site, 'T' might bind with same force as 'const T &'
+
+void compare05(T t1, const T &t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'compare05' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:31: note: last argument in the adjacent range
+// CHECK-MESSAGES: :[[@LINE-3]]:22: note: at a call site, 'const T &' might bind with same force as 'T'
+
+void compare06(const T &t1, const T &t2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'compare06' of similar type ('const T &') are
+// CHECK-MESSAGES: :[[@LINE-2]]:38: note: last argument in the adjacent range
+
+void compare07(const T &t1, const

[PATCH] D69560: [clang-tidy] Add 'cppcoreguidelines-avoid-adjacent-arguments-of-same-type' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.
Herald added a subscriber: wuzish.

A few interesting "true positive" findings:

- F10568770: 
AnalysisDeclContext.cpp_9aaed563ddd9ebd73fdd228c2883b8e7.plist.html 
 (Such cases with many `bool`s are being 
discussed on enhancing type safety and type strength 

- F10568771: bitcoingui.cpp_27769deaa63c894e4fc430d7122c368d.plist.html 

- F10568773: CheckerRegistry.cpp_d0d988840a154a07670cc410e23a7c8e.plist.html 

- F10568775: text_format.cc_f34502df2725b30018c086b6767cbd26.plist.html 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560



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

[PATCH] D69560: [clang-tidy] Add 'cppcoreguidelines-avoid-adjacent-arguments-of-same-type' check

[Whisperity via Phabricator via cfe-commits]

whisperity added reviewers: Szelethus, baloghadamsoftware.
whisperity edited subscribers, added: baloghadamsoftware, NoQ; removed: 
Szelethus.
whisperity added a comment.

@Szelethus and @baloghadamsoftware are colleagues to me whom are far more 
knowledgeable about check development and I want them to see that I want a 
review from them. I specifically didn't do an "internal with colleagues" 
downstream review with regards to this code.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp:488
+void AdjacentArgumentsOfSameTypeCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+return;

Eugene.Zelenko wrote:
> Check seems to be useful for C and probably for Objective-C.
I'm not knowledgeable about Objective-C at all to make a decision on how the 
"adjacent argument ranges" could be calculated and what mixups are possible. As 
for C, should a `cppcoreguidelines-` check be enabled for C? Or you mean we 
should allow it to work, and the user will toggle how they see fit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560



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

[PATCH] D75665: [analyzer] On-demand parsing capability for CTU

[Whisperity via Phabricator via cfe-commits]

whisperity resigned from this revision.
whisperity added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:26
+#include "llvm/ADT/Triple.h"
+#include "llvm/Option/ArgList.h"
 #include "llvm/Support/ErrorHandling.h"

Duped include in L34.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75665



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

[PATCH] D84176: [CMake][CTE] Add "check-clang-tools-..." targets to test only a particular Clang extra tool

[Whisperity via Phabricator via cfe-commits]

whisperity created this revision.
whisperity added a reviewer: klimek.
whisperity added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, martong, gamesh411, Szelethus, dkrupp, 
rnkovacs, mgorny.
Herald added a project: clang.

Create targets `check-clang-tools-clang-tidy`, `check-clang-tools-clang-query` 
similar to how `check-clang-sema`, `check-clang-parser`, etc. are 
auto-generated from the directory structure.

This allows running only a particular sub-tool's tests, not having to wait 
through the //entire// `check-clang-tools`'s execution.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84176

Files:
  clang-tools-extra/test/CMakeLists.txt


Index: clang-tools-extra/test/CMakeLists.txt
===
--- clang-tools-extra/test/CMakeLists.txt
+++ clang-tools-extra/test/CMakeLists.txt
@@ -90,3 +90,7 @@
   )
 
 set_target_properties(check-clang-tools PROPERTIES FOLDER "Clang extra tools' 
tests")
+
+add_lit_testsuites(CLANG-TOOLS ${CMAKE_CURRENT_SOURCE_DIR}
+  DEPENDS ${CLANG_TOOLS_TEST_DEPS}
+  )


Index: clang-tools-extra/test/CMakeLists.txt
===
--- clang-tools-extra/test/CMakeLists.txt
+++ clang-tools-extra/test/CMakeLists.txt
@@ -90,3 +90,7 @@
   )
 
 set_target_properties(check-clang-tools PROPERTIES FOLDER "Clang extra tools' tests")
+
+add_lit_testsuites(CLANG-TOOLS ${CMAKE_CURRENT_SOURCE_DIR}
+  DEPENDS ${CLANG_TOOLS_TEST_DEPS}
+  )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84176: [CMake][CTE] Add "check-clang-tools-..." targets to test only a particular Clang extra tool

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@lebedev.ri If you have an idea on who's competent in accepting this change, 
please update the //reviewers// field, I'm in the dark here.

Also, should the commands be called `check-clang-tools-clang-tidy` or 
`check-clang-`**`extra`**`-clang-tidy` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84176



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

[PATCH] D77150: [Analyzer] New Option for ContainerModeling: AggressiveEraseModeling

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D77150#2149870 , @dkrupp wrote:

> Since the analyzer cannot cannot model the size of the containers just yet 
> (or precisely enough), what we are saying with this checker is to "always 
> check the return value of the erase() function before use (for 
> increment/decrement etc.) whether it is not past-end" .
>
> Adam, are you sure that the user would like to enforce such a generic coding 
> rule? Depending on the actual code analyzed, this would require this clumsy 
> check at potentially too many places.

While I agree that realising in the analyser that the user code at hand is 
meant to express that the iterator stepping will not go out of bounds, you have 
to keep in mind, that going out of bounds with an iterator is still UB. In case 
of a `for` loop, it's //always// a dangerous construct to call `erase()` 
inside, unless you, indeed, can explicitly ensure that you won't go out of 
bounds.

Side note: I am surprised there isn't a Tidy check in `bugprone-` that simply 
says //"If you are using `erase()`, use a `while` loop and not a `for` 
loop..."//

I'm not terribly up-to-date with @baloghadamsoftware's patches (nor the innards 
of the analyser), but as a user who'd get this warning, I believe one crucial 
information missing is something like //"note: assuming `erase()` removed the 
the last element, setting `it` to the past-the-end iterator"//

In D77150#2149870 , @dkrupp wrote:

> Oops, I did not mean "request changes". This is just meant to be an 
> additional comment in this discussion.

You can do the //Resign as Reviewer// action to remove your `X` from the patch. 
🙂




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:620-624
+  "AggressiveEraseModeling",
+  "Enables exploration of the past-the-end branch for the "
+  "return value of the erase() method of containers.",
+  "false",
+  Released>

Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > Hmm. The description isn't really user-friendly, imo, but the option 
> > > doesn't sound too user-facing either. I don't think this is an option to 
> > > be tinkered with. I think we should make this hidden.
> > > 
> > > Also, even for developers, I find this a bitconfusing at first. Do we not 
> > > enable that by default? Why not? What do we have to gain/lose?
> > What is the rule that the user needs to follow in order to avoid the 
> > warning? "Always check the return value of `erase()` before use"? If so, a 
> > good description would be along the lines of "Warn when the return value of 
> > `erase()` is not checked before use; compare that value to end() in order 
> > to suppress the warning" (or something more specific).
> Please mark this hidden.
Is marking something hidden still allow for modifying the value from the 
command-line, or via CodeChecker?



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:708-709
+  // end symbol from the container data because the  past-the-end iterator
+  // was also invalidated. The next call to member function `end()` would
+  // create a new one, but we need it now so we create it now.
+  if (!EndSym) {

Is this creation prevented?


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

https://reviews.llvm.org/D77150

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

[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:19
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "llvm/Config/config.h"
+#include "gtest/gtest.h"

steakhal wrote:
> mgorny wrote:
> > `config.h` is a private LLVM header and must not be used from clang.
> I'm not a CMake profession, but shouldn't it be declared private to the LLVM 
> target in the corresponding CMakeLists file then?
> 
> How do you query whether clang has built with Z3 or not @mgorny?
> I'm including that header only for that.
I've did a quick skim of the code, and it seems there is no way in it currently 
to query this.
Your best bet would be either adding an extra flag to Clang's CMake generated 
Config header that inherits this flag, or checking `llvm::CreateZ3Solver()` - 
right now, this method reports a fatal error, but you could create a bool 
function in the header which reports constant true or false, and turn the 
test's skip into a runtime condition?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78704

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

[PATCH] D84176: [CMake][CTE] Add "check-clang-extra-..." targets to test only a particular Clang extra tool

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 281922.
whisperity retitled this revision from "[CMake][CTE] Add 
"check-clang-tools-..." targets to test only a particular Clang extra tool" to 
"[CMake][CTE] Add "check-clang-extra-..." targets to test only a particular 
Clang extra tool".
whisperity added reviewers: chandlerc, alexfh.
whisperity added a comment.

After using this for a while, it feels more natural and more obvious that these 
are the CTE targets, not the "clang/tools" test targets.


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

https://reviews.llvm.org/D84176

Files:
  clang-tools-extra/test/CMakeLists.txt


Index: clang-tools-extra/test/CMakeLists.txt
===
--- clang-tools-extra/test/CMakeLists.txt
+++ clang-tools-extra/test/CMakeLists.txt
@@ -90,3 +90,7 @@
   )
 
 set_target_properties(check-clang-tools PROPERTIES FOLDER "Clang extra tools' 
tests")
+
+add_lit_testsuites(CLANG-EXTRA ${CMAKE_CURRENT_SOURCE_DIR}
+  DEPENDS ${CLANG_TOOLS_TEST_DEPS}
+  )


Index: clang-tools-extra/test/CMakeLists.txt
===
--- clang-tools-extra/test/CMakeLists.txt
+++ clang-tools-extra/test/CMakeLists.txt
@@ -90,3 +90,7 @@
   )
 
 set_target_properties(check-clang-tools PROPERTIES FOLDER "Clang extra tools' tests")
+
+add_lit_testsuites(CLANG-EXTRA ${CMAKE_CURRENT_SOURCE_DIR}
+  DEPENDS ${CLANG_TOOLS_TEST_DEPS}
+  )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75665: [analyzer] On-demand parsing capability for CTU

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:119-120
+  return "Invocation list file contains multiple references to the same "
+ "source"
+ " file.";
+case index_error_code::invocation_list_file_not_found:

These two lines could be formatted together.



Comment at: clang/unittests/CrossTU/CrossTranslationUnitTest.cpp:233
+  EXPECT_EQ(It->getValue()[4], "/tmp/other.cpp");
 }
 

Just for good measure, I would like a test case on the "invocation list is 
ambiguous" error. (So in case we in the future figure out how to disambiguate, 
we can remove that test case!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75665



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

[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In general, the test files should be cleaned up.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-in-multithreaded-program-config-std::thread.cpp:1
+// RUN: %check_clang_tidy %s bugprone-signal-in-multithreaded-program %t \
+// RUN: -config='{CheckOptions: \

Please rename this file to something that doesn't conatain `:` in the name. 
Windows peoples' computers will explode if it's merged like this...



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-in-multithreaded-program-config-std::thread.cpp:5-6
+
+typedef unsigned long int thrd_t;
+typedef int (*thrd_start_t)(void *);
+typedef int sig_atomic_t;

Are these definitions needed for C++ test?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-in-multithreaded-program-config-std::thread.cpp:32
+
+int main(void) {
+  signal(SIGUSR1, handler);

As far as I know, `int main(void)` is **not** valid C++ code. Only `int main()` 
and `int main(int, char**)` are valid, see `[basic.start.main]`/2.

If we are writing C++ test code, let's make it appear as if it was C++ code.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-in-multithreaded-program-noconfig-std::thread.cpp:1
+// RUN: %check_clang_tidy %s bugprone-signal-in-multithreaded-program %t
+

Same rename here.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-in-multithreaded-program-noconfig-thrd_create.cpp:1
+// RUN: %check_clang_tidy %s bugprone-signal-in-multithreaded-program %t
+

thrd_create is a C standard function, and this file looks like a C file to me, 
so why is the extension `.cpp`?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D75229



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

[PATCH] D77493: [clang-tidy] Add do-not-refer-atomic-twice check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DoNotReferAtomicTwiceCheck.cpp:38
+  diag(MatchedRef->getExprLoc(),
+   "Do not refer to '%0' atomic variable twice in an expression")
+  << MatchedVar->getName();

atomic variable '%0'



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:82
+
+  Finds atomic variable which is referred twice in an expression.
+

atomic variable**s** which **are**


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D77493



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

[PATCH] D79437: [clang-tidy] Add fsetpos argument checker

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-fsetpos-argument-check.cpp:1
+// RUN: %check_clang_tidy %s bugprone-fsetpos-argument-check %t
+

This is a C, not a C++ file, the extension should show it. In addition, a 
similar test should be added that uses `std::fgetpos()` and `std::fsetpos()`, 
and shows the matching still works.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D79437



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

[PATCH] D81272: [clang-tidy] New check `misc-redundant-condition`

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

As a future work, when something support `if`s, it should also support `?:`, 
and eliminate redundant conditionals from there, too.

I believe this check (together with `misc-redundant-expr`) should go into the 
`readability-` group.


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

https://reviews.llvm.org/D81272



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

[PATCH] D75665: [analyzer] On-demand parsing capability for CTU

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:21
+
+PCH based analysis
+__

I think it's PCH-based with a -.



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:25
+These can be generated by the Clang Frontend itself, and must be arranged in a 
specific way in the filesystem.
+The index, which maps function USR names to PCH dumps containing them must 
also be generated by the
+`clang-extdef-mapping` clang tool. The external mapping creation implicitly 
uses a compilation command database to

symbols' USR names



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:26
+The index, which maps function USR names to PCH dumps containing them must 
also be generated by the
+`clang-extdef-mapping` clang tool. The external mapping creation implicitly 
uses a compilation command database to
+determine the compilation flags used.

martong wrote:
> Maybe just simply write `This tool uses a compilation command database ...`
> (Same below with the on-demand version.)
~~clang tool~~



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:26
+The index, which maps function USR names to PCH dumps containing them must 
also be generated by the
+`clang-extdef-mapping` clang tool. The external mapping creation implicitly 
uses a compilation command database to
+determine the compilation flags used.

whisperity wrote:
> martong wrote:
> > Maybe just simply write `This tool uses a compilation command database ...`
> > (Same below with the on-demand version.)
> ~~clang tool~~
And perhaps add some cross-references to what a compilation database is, etc. 
These things are also documented within Clang's doc tree.



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:31
+
+[PCH] Manual CTU Analysis
+-

Instead of a prefix, create an overview of PCH-based analysis, and add this and 
the next one as a 3rd-level heading?



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:111-114
+  -Xclang -analyzer-config -Xclang 
experimental-enable-naive-ctu-analysis=true \
+  -Xclang -analyzer-config -Xclang ctu-dir=. \
+  -Xclang -analyzer-config -Xclang ctu-on-demand-parsing=false \
+  -Xclang -analyzer-output=plist-multi-file \

Are these flags documented somewhere?



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:227-228
+compilation database in order to determine the exact compiler invocation used 
for each TU.
+The index, which maps function USR names to source files containing them must 
also be generated by the
+`clang-extdef-mapping` clang tool. The mapping of external definitions 
implicitly uses a compilation command database to
+determine the compilation flags used. Preferably the same compilation database 
should be used when generating the

Same here for comments from above.



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:331-335
+  [INFO 2019-07-16 17:21] - To view results in the terminal use the 
"CodeChecker parse" command.
+  [INFO 2019-07-16 17:21] - To store results use the "CodeChecker store" 
command.
+  [INFO 2019-07-16 17:21] - See --help and the user guide for further options 
about parsing and storing the reports.
+  [INFO 2019-07-16 17:21] - =
+  [INFO 2019-07-16 17:21] - Analysis length: 0.659618854523 sec.

These lines could be removed, I think, they don't add anything to the 
presentation.



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:336-337
+  [INFO 2019-07-16 17:21] - Analysis length: 0.659618854523 sec.
+  $ ls
+  compile_commands.json  foo.cpp main.cpp  reports
+  $ tree reports

Due to the lack of colours, perhaps you could use `ls -F` which suffixes each 
directory with a `/`: `reports/`.



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:387
+-
+We actively develop CTU with CodeChecker as a "runner" script, `scan-build-py` 
is not actively developed for CTU.
+`scan-build-py` has various errors and issues, expect it to work with the very 
basic projects only.

as ~~a runner script~~ the driver for this feature



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:388
+We actively develop CTU with CodeChecker as a "runner" script, `scan-build-py` 
is not actively developed for CTU.
+`scan-build-py` has various errors and issues, expect it to work with the very 
basic projects only.
+

expect it to work only ...

(to emphasise the limitation)



Comment at: clang/lib/CrossTU/Cr

[PATCH] D78652: [clang-tidy] Add "ignore related parameters" heuristics to 'experimental-cppcoreguidelines-avoid-adjacent-arguments-of-the-same-type'

[Whisperity via Phabricator via cfe-commits]

whisperity created this revision.
whisperity added reviewers: aaron.ballman, alexfh, hokein, JonasToth, zporky.
whisperity added projects: clang-tools-extra, clang.
Herald added subscribers: cfe-commits, martong, gamesh411, dkrupp, rnkovacs, 
kbarton, nemanjai.
whisperity added a parent revision: D69560: [clang-tidy] Add 
'experimental-cppcoreguidelines-avoid-adjacent-arguments-of-the-same-type' 
check.
Herald added a subscriber: wuzish.

The basic version of the checker only considers the pseudo-equality between 
types of parameters as the predicate on deciding whether or not the two can be 
mixed up with each other at a potential call site. This, together with a low 
minimum range length requirement, results in an incredibly verbose checker with 
report count potentially in the multiple thousands.

This patch aims to mitigate that problem by silencing warnings about adjacent 
parameter ranges in which the parameters are //related// to each other.
While this change definitely brings in false negatives (as mixing the arguments 
to `max(a, b)` is still a potential issue, but we really can't do better in 
that case...), the sheer amount of reduction in the report count should make up 
for it. Across multiple projects (from both C and C++), the number of reports 
has dropped by **at least 40%**.
This should help to make sure that the dodgiest interfaces are not hidden under 
clumps of naively generated reports.

Currently, three relatedness heuristics are implemented, each in a way that it 
is easy to, in the future, remove them or add new ones.

- Common expression: if both parameters are found in a common expression 
somewhere in the function, they are considered related. This heuristic is an 
absolute blast because it deals with arithmetics, comparisons, and forwarding 
functions all in one.
- Return: if both parameters are returned on different execution paths of the 
function, they are considered related. This deals with switch-like functions.
- Pass to same function: if both parameters are passed to the same function in 
the same index, they are considered related. This is a special case of 
forwarding. I.e. for `a` and `b` if there exists a call `f(foo, a);` and 
`f(bar, b);`, they are deemed related. Thanks to @zporky, our C++ expert, the 
idea.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78652

Files:
  
clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp
  
clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-cvr-on.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-relatedness.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c

Index: clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
===
--- clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
+++ clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
@@ -1,7 +1,8 @@
 // RUN: %check_clang_tidy %s \
 // RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
 // RUN:   -config='{CheckOptions: [ \
-// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1} \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1}, \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CheckRelatedness, value: 0} \
 // RUN:   ]}' --
 
 struct T {};
Index: clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-relatedness.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-relatedness.cpp
@@ -0,0 +1,187 @@
+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CheckRelatedness, value: 1} \
+// RUN:   ]}' --
+
+namespace std {
+// NO-WARN: Not a definition, the user has no chance to fix this!
+int max(int x, int y);
+int abs(int x);
+
+template 
+bool less(const T &t1, const T &t2);
+} // namespace std
+
+class C {
+public:
+  void foo(int a);
+  void bar(int a, int b);
+};
+
+bool coin();
+int f(int a);
+int g(int b);
+int h(int

[PATCH] D69560: [clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-arguments-of-the-same-type' check

[Whisperity via Phabricator via cfe-commits]

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

- Better organisation of code, cleanup of debug prints, fix nomenclature of 
functions
- Explicitly mention the first and last param of the range for clarity
- Downlift the logic of joining and breaking ranges to main patch (this isn't 
terribly useful at this point, but both subsequent improvements to the check 
depend on this change)
  - Basically no longer assume that if param N can be swapped with param 1, 
then it can also be swapped with 2, 3, etc. without checking.
- Made the list of ignored parameter and type names configurable as a checker 
option
- Changed the default value of `MinimumLength` to `2`, as prescribed in the 
guideline's text.


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

https://reviews.llvm.org/D69560

Files:
  clang-tools-extra/clang-tidy/experimental/CMakeLists.txt
  
clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp
  
clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h
  clang-tools-extra/clang-tidy/experimental/ExperimentalTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-cvr-on.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-ignores.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-minlen3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c

Index: clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
@@ -0,0 +1,34 @@
+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1} \
+// RUN:   ]}' --
+
+struct T {};
+
+void memcpy(struct T *restrict dest, const struct T *restrict src) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters for 'memcpy' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in this range is 'dest'
+// CHECK-MESSAGES: :[[@LINE-3]]:63: note: the last parameter in this range is 'src'
+// CHECK-MESSAGES: :[[@LINE-4]]:38: note: at a call site, 'const struct T * restrict' might bind with same force as 'struct T *restrict'
+
+void merge(struct T *dst, const struct T *src1, const struct T *src2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 3 adjacent parameters for 'merge' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:22: note: the first parameter in this range is 'dst'
+// CHECK-MESSAGES: :[[@LINE-3]]:65: note: the last parameter in this range is 'src2'
+// CHECK-MESSAGES: :[[@LINE-4]]:27: note: at a call site, 'const struct T *' might bind with same force as 'struct T *'
+// CHECK-MESSAGES: :[[@LINE-5]]:49: note: at a call site, 'const struct T *' might bind with same force as 'struct T *'
+
+int equals(struct T a, struct T b) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 2 adjacent parameters for 'equals' of similar type ('struct T') are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in this range is 'a'
+// CHECK-MESSAGES: :[[@LINE-3]]:33: note: the last parameter in this range is 'b'
+
+typedef struct {
+  int x;
+} S;
+
+int equals2(S l, S r) { return l.x == r.x; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters for 'equals2' of similar type ('S') are
+// CHECK-MESSAGES: :[[@LINE-2]]:15: note: the first parameter in this range is 'l'
+// CHECK-MESSAGES: :[[@LINE-3]]:20: note: the last parameter in this range is 'r'
Index: clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-minlen3.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-minlen3.cpp
@@ -0,0 +1,205 @@
+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.MinimumLength, value: 3} \
+// RUN:

[PATCH] D75041: [clang-tidy] Approximate implicit conversion issues in 'experimental-cppcoreguidelines-avoid-adjacent-arguments-of-the-same-type'

[Whisperity via Phabricator via cfe-commits]

whisperity marked 2 inline comments as done.
whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp:747
 OS << "...";
-  } else
+  } else {
 // There are things like "GCC Vector type" and such that who knows how

'chute, I hate merge conflicts...



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.rst:113-120
+.. warning::
+Turning the modelling of implicit conversion sequences on
+relaxes the constraints for "type convertibility" significantly,
+however, it also applies a generous performance hit on the check's 
cost.
+The check will have to explore a **polynomially more** possibilities:
+O(n\ :sup:`2`\ ) instead of O(n) for each function's ``n`` parameters.
+The emitted diagnostics will also be more verbose, which might take 
more

This change to check the "left half" of a full graph moved to the main checker 
patch D69560 and there is no significant (few seconds, on large projects like 
LLVM) time difference between the modes at all even on large projects... so 
this fearmongering text should be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041



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

[PATCH] D69560: [clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-arguments-of-the-same-type' check

[Whisperity via Phabricator via cfe-commits]

whisperity marked an inline comment as done.
whisperity added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.rst:136-139
+typedef   T  element_type;
+typedef const Tconst_element_type;
+typedef   T &reference_type;
+typedef const T &  const_reference_type;

Why are these typos still here, `typedef`??? instead of `typename`.


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

https://reviews.llvm.org/D69560



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

[PATCH] D75041: [clang-tidy] Approximate implicit conversion issues in 'experimental-cppcoreguidelines-avoid-adjacent-arguments-of-the-same-type'

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 259324.
whisperity retitled this revision from "[clang-tidy] Approximate implicit 
conversion issues for the 
'experimental-cppcoreguidelines-avoid-adjacent-arguments-of-the-same-type' 
check" to "[clang-tidy] Approximate implicit conversion issues in 
'experimental-cppcoreguidelines-avoid-adjacent-arguments-of-the-same-type'".
whisperity added a comment.

- Re-organised code, removed debug prints, rebased, the usual tidy-up.
- Bug fixes on certain crashes like incomplete types, conversion operator 
templates, etc.
- Even more bug fixes against crashes, hopefully... sorry I lost the individual 
commits in a `squash` I think


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

Files:
  
clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp
  
clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.rst
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-implicits.c
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-implicits.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-minlen3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c

Index: clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
===
--- clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
+++ clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
@@ -32,3 +32,6 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters for 'equals2' of similar type ('S') are
 // CHECK-MESSAGES: :[[@LINE-2]]:15: note: the first parameter in this range is 'l'
 // CHECK-MESSAGES: :[[@LINE-3]]:20: note: the last parameter in this range is 'r'
+
+void ptrs(int *i, long *l) {}
+// NO-WARN: Mixing fundamentals' pointers is diagnosed by compiler warnings.
Index: clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-minlen3.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-minlen3.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-minlen3.cpp
@@ -203,3 +203,20 @@
 // CHECK-MESSAGES: :[[@LINE-6]]:25: note: after resolving type aliases, type of parameter 'Background' is 'OldSchoolTermColour'
 // CHECK-MESSAGES: :[[@LINE-6]]:25: note: after resolving type aliases, type of parameter 'Foreground' is 'OldSchoolTermColour'
 // CHECK-MESSAGES: :[[@LINE-6]]:25: note: after resolving type aliases, type of parameter 'Border' is 'OldSchoolTermColour'
+
+// NO-WARN: Implicit conversions should not warn if the check option is turned off.
+
+void integral1(signed char sc, int si) {}
+
+struct FromInt {
+  FromInt(int);
+};
+void userConv1(int i, FromInt fi) {}
+
+struct Base {};
+struct Derived1 : Base {};
+struct Derived2 : Base {};
+void upcasting(Base *bp, Derived1 *d1p, Derived2 *d2p) {}
+void upcasting_ref(const Base &br, const Derived1 &d1r, const Derived2 &d2r) {}
+
+// END of NO-WARN.
Index: clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-implicits.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-implicits.cpp
@@ -0,0 +1,362 @@
+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.ImplicitConversion, value: 1} \
+// RUN:   ]}' --
+
+void compare(int a, int b, int c) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 3 adjacent parameters for 'compare' of similar type ('int') are easily swapped by mistake [experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in this range is 'a'
+// CHECK-MESSAGES: :[[@LINE-3]]:32: note: the last parameter in this range is 'c'
+
+void b(bool b1, bool b2, int i) {}
+// CHECK-MESSAGES: :[

[PATCH] D75665: [analyzer] On-demand parsing capability for CTU

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang/docs/analyzer/user-docs/CrossTranslationUnit.rst:391
+Currently On-demand analysis is not supported with `scan-build-py`.
\ No newline at end of file


What's this line? Is this added by Phab, or is this a weird merge conflict / 
parsing error of the patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75665



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

[PATCH] D20689: [clang-tidy] Add 'readability-suspicious-call-argument' check

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 259508.
whisperity retitled this revision from "[clang-tidy] Suspicious Call Argument 
checker" to "[clang-tidy] Add 'readability-suspicious-call-argument' check".
whisperity edited the summary of this revision.
whisperity removed reviewers: varjujan, barancsuk.
whisperity set the repository for this revision to rG LLVM Github Monorepo.
whisperity edited subscribers, added: varjujan, zporky; removed: gsd.
whisperity added a comment.
Herald added a project: clang.

First things first, we were 50 thousand (!) patches behind reality. Rebased to 
`master`. Fixed it to compile, too. Otherwise, NFC so far.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D20689

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
  clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/readability-suspicious-call-argument.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
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-suspicious-call-argument.cpp
@@ -0,0 +1,392 @@
+// RUN: %check_clang_tidy %s readability-suspicious-call-argument %t -- -- -std=c++11
+
+void foo_1(int aa, int bb) {}
+
+void foo_2(int source, int aa) {}
+
+void foo_3(int valToRet, int aa) {}
+
+void foo_4(int pointer, int aa) {}
+
+void foo_5(int aa, int bb, int cc, ...) {}
+
+void foo_6(const int dd, bool &ee) {}
+
+void foo_7(int aa, int bb, int cc, int ff = 7) {}
+
+// Test functions for convertible argument--parameter types.
+void fun(const int &m);
+void fun2() {
+  int m = 3;
+  fun(m);
+}
+
+// Test cases for parameters of const reference and value.
+void value_const_reference(int ll, const int &kk);
+
+void const_ref_value_swapped() {
+  const int &kk = 42;
+  const int &ll = 42;
+  value_const_reference(kk, ll);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: kk (ll) is swapped with ll (kk). [readability-suspicious-call-argument]
+}
+
+// Const, non const references.
+void const_nonconst_parameters(const int &mm, int &nn);
+
+void const_nonconst_swap1() {
+  const int &nn = 42;
+  int mm;
+  // Do not check, because non-const reference parameter cannot bind to const reference argument.
+  const_nonconst_parameters(nn, mm);
+}
+
+void const_nonconst_swap3() {
+  const int nn = 42;
+  int m = 42;
+  int &mm = m;
+  // Do not check, const int does not bind to non const reference.
+  const_nonconst_parameters(nn, mm);
+}
+
+void const_nonconst_swap2() {
+  int nn;
+  int mm;
+  // Check for swapped arguments. (Both arguments are non-const.)
+  const_nonconst_parameters(
+  nn,
+  mm);
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: nn (mm) is swapped with mm (nn). [readability-suspicious-call-argument]
+}
+
+void const_nonconst_pointers(const int *mm, int *nn);
+void const_nonconst_pointers2(const int *mm, const int *nn);
+
+void const_nonconst_pointers_swapped() {
+  int *mm;
+  const int *nn;
+  const_nonconst_pointers(nn, mm);
+}
+
+void const_nonconst_pointers_swapped2() {
+  const int *mm;
+  int *nn;
+  const_nonconst_pointers2(nn, mm);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: nn (mm) is swapped with mm (nn). [readability-suspicious-call-argument]
+}
+
+// Test cases for pointers and arrays.
+void pointer_array_parameters(
+int *pp, int qq[4]);
+
+void pointer_array_swap() {
+  int qq[5];
+  int *pp;
+  // Check for swapped arguments. An array implicitly converts to a pointer.
+  pointer_array_parameters(qq, pp);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: qq (pp) is swapped with pp (qq). [readability-suspicious-call-argument]
+}
+
+// Test cases for multilevel pointers.
+void multilevel_pointer_parameters(int *const **pp,
+   const int *const *volatile const *qq);
+void multilevel_pointer_parameters2(
+char *nn, char *volatile *const *const *const *const &mm);
+
+typedef float T;
+typedef T *S;
+typedef S *const volatile R;
+typedef R *Q;
+typedef Q *P;
+typedef P *O;
+void multilevel_pointer_parameters3(float **const volatile ***rr, O &ss);
+
+void multilevel_pointer_swap() {
+  int *const **qq;
+  int *const **pp;
+  multilevel_pointer_parameters

[PATCH] D20689: [clang-tidy] Add 'readability-suspicious-call-argument' check

[Whisperity via Phabricator via cfe-commits]

whisperity commandeered this revision.
whisperity added a reviewer: barancsuk.
whisperity added a comment.
Herald added subscribers: martong, Charusso, gamesh411, Szelethus.

Assuming direct control. Previous colleagues and university mates departed for 
snowier pastures, time to try to do something with this check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D20689



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

[PATCH] D78457: [analyzer][Z3-refutation] Fix refutation BugReporterVisitor bug and refactor

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@steakhal You might want to update the patch summary before committing this to 
the upstream (it still mentions "not needing a visitor" 😄)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78457



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

[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:24
+  do   
\
+if (!LLVM_WITH_Z3) 
\
+  return;  
\

xazax.hun wrote:
> steakhal wrote:
> > xazax.hun wrote:
> > > I think this might not be the idiomatic way to skip a test. Consider 
> > > using ` GTEST_SKIP();`.
> > I agree, though that is not yet supported in the `gtest` in the repository.
> > Should we update that to benefit from this macro?
> > 
> > There are several places where we could use that like:
> > - 
> > [llvm/unittests/ExecutionEngine/MCJIT/MCJITTestAPICommon.h:29](https://github.com/llvm/llvm-project/blob/master/llvm/unittests/ExecutionEngine/MCJIT/MCJITTestAPICommon.h#L29-L33)
> >  (expanded 37 times in the codebase)
> > - 
> > [llvm/unittests/Support/ThreadPool.cpp:81](https://github.com/llvm/llvm-project/blob/master/llvm/unittests/Support/ThreadPool.cpp#L81-L85)
> >  (expanded 7 times in the file)
> > - 
> > [llvm/unittests/Support/MemoryTest.cpp:89](https://github.com/llvm/llvm-project/blob/master/llvm/unittests/Support/MemoryTest.cpp#L89-L94)
> >  (expanded 10 time in the file)
> I think you should check who updated gtest the last time and ping them what 
> is the process to update it again.
Still maybe this could warrant a `// FIXME: Update GTest and use GTEST_SKIP()` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78704



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

[PATCH] D83717: [clang-tidy] Add check fo SEI CERT item ENV32-C

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IsRegisterFunction =

What happens if this test is run on C++ code calling the same functions? I see 
the rule is only applicable to C, for some reason... Should it be disabled from 
registering if by accident the check is enabled on a C++ source file?



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:68-71
+  0, declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")))
+ .bind("handler_expr"));
+  Finder->addMatcher(
+  callExpr(IsRegisterFunction, HasHandlerAsFirstArg).bind("register_call"),

It is customary in most Tidy checks that use multiple binds to have the bind 
names defined as a symbol, instead of using just two string literals as if the 
bind has to be renamed, it's easy to mess it up.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:112-113
+  diag(RegisterCall->getBeginLoc(),
+   "exit-handler potentially calls an exit function. Handlers should "
+   "terminate by returning");
+  diag(HandlerDecl->getBeginLoc(), "handler function declared here",

Same as below, suggestion: "exit hander potentially calls exit function instead 
of terminating normally with a return".

("exit handler" and "exit function" without - is more in line with the SEI CERT 
rule's phrasing too, they don't say "exit-handler".)



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:124-125
+  diag(RegisterCall->getSourceRange().getBegin(),
+   "exit-handler potentially calls a jump function. Handlers should "
+   "terminate by returning");
+  diag(HandlerDecl->getBeginLoc(), "handler function declared here",

This semi-sentence structure of starting with lowercase but also terminating 
the sentence and leaving in another but unterminated sentences looks really odd.

Suggestion: "exit handler potentially jumps instead of terminating normally 
with a return"



Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c:31
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];

Which is the standard version this test file is set to analyse with? I don't 
see any `-std=` flag in the `RUN:` line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

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

[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D75229#2189666 , @abelkocsis wrote:

> I have managed to improve the checker, but could not set up the test files to 
> work only in POSIX platforms. Could you help me or show me an example?

It's more so that, if I understand correctly, you **don't** want the 
diagnostics to be emitted when dealing with POSIX multithreading.

Also, have you checked in the test if `__unix__` is defined? I'm not seeing any 
indication of targets in the test configuration, nor checking for this macro. 
Maybe a Windows-specific test file should be added? 
`clang/test/Preprocessor/init.c` lists a bunch of macros that are supposedly 
pre-defined based on the target.

Or maybe the `OSType` in the triple would be more useful than the vendor?




Comment at: 
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:68
+const SourceManager &SM, Preprocessor *pp, Preprocessor *ModuleExpanderPP) 
{
+  PP = pp;
+}

Global state is always a potential problem. Are you sure this is the right way 
to do things? Most other tidy checks that actively use this function define 
their own `PPCallback` subclass and use the existing preprocessor here to 
ensure the preprocessor executes the callback.



Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:50-85
 "cert-con54-cpp");


Please upload the diff with full context.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:102
`bugprone-virtual-near-miss `_, "Yes"
+   `cert-con37-c `_,
`cert-dcl21-cpp `_,

There is a table in this file towards the end where alias checks are explicitly 
listed in a "which check is alias of which other check" format.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D75229

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

[PATCH] D83717: [clang-tidy] Add check fo SEI CERT item ENV32-C

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:33
+bool isExitFunction(StringRef FnName) {
+  return FnName == "_Exit" || FnName == "exit" || FnName == "quick_exit";
+}

aaron.ballman wrote:
> Because this rule applies in C++ as well as C, you should protect these names 
> so that calling something like this doesn't trigger the check:
> ```
> namespace menu_items {
> void exit(int);
> }
> ```
> I think we want `::_Exit` and `::std::_Exit` to check the fully qualified 
> names (I believe this will still work in C, but should be tested). The same 
> goes for the other names (including `atexit` and `at_quick_exit` above).
> I think we want `::_Exit` and `::std::_Exit` to check the fully qualified 
> names (I believe this will still work in C, but should be tested).

Tested with Clang-Query:

```
clang-query> m functionDecl(hasName("::atexit"))

Match #1:

/home/whisperity/LLVM/Build/foo.c:1:1: note: "root" binds here
int atexit(int) {}
^~
1 match.
```



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IsRegisterFunction =

aaron.ballman wrote:
> whisperity wrote:
> > What happens if this test is run on C++ code calling the same functions? I 
> > see the rule is only applicable to C, for some reason... Should it be 
> > disabled from registering if by accident the check is enabled on a C++ 
> > source file?
> The CERT C++ rules inherit many of the C rules, including this one. It's 
> listed towards the bottom of the set of inherited rules here: 
> https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336
Right, thanks for the heads-up. This should be somewhat more indicative on the 
Wiki on the page for the rule (especially because people won't immediately 
understand why a `-c` check reports on their cpp code, assuming they understand 
`-c` means C.)



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:124-125
+  diag(RegisterCall->getSourceRange().getBegin(),
+   "exit-handler potentially calls a jump function. Handlers should "
+   "terminate by returning");
+  diag(HandlerDecl->getBeginLoc(), "handler function declared here",

aaron.ballman wrote:
> whisperity wrote:
> > This semi-sentence structure of starting with lowercase but also 
> > terminating the sentence and leaving in another but unterminated sentences 
> > looks really odd.
> > 
> > Suggestion: "exit handler potentially jumps instead of terminating normally 
> > with a return"
> Slight tweak here as well. I don't think we'll ever see a jump function other 
> than longjmp for this rule, so what about writing `potentially calls 
> 'longjmp'` instead of `potentially jumps`?
😉 Agree. And if we see, we will have to update the code anyways. One could in 
theory whip up some inline assembly black magic, but that's a whole other can 
of worms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

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