[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Apart from those in the in-line comments, I have a question: how safe is this 
library to `Release` builds? I know this is only a submodule dependency for the 
"real deal" in https://reviews.llvm.org/D30691, but I have seen some asserts 
that "imported function should already have a body" and such.

Will the static analyzer handle these errors gracefully and fall back to 
"function is unknown, let's throw my presumptions out of the window" or will it 
bail away? I'm explicitly thinking of the assert-lacking `Release` build.




Comment at: include/clang/Basic/AllDiagnostics.h:20
 #include "clang/AST/CommentDiagnostic.h"
+#include "clang/CrossTU/CrossTUDiagnostic.h"
 #include "clang/Analysis/AnalysisDiagnostic.h"

Import file order?



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:42
+/// Note that this class also implements caching.
+class CrossTranslationUnit {
+public:

Does the name of this class make sense? If I say


```
CrossTranslationUnit *ctuptr = new CrossTranslationUnit(...);
```

did I construct a new //CrossTranslationUnit//? What even **is** a 
//CrossTranslationUnit//? Other class names, such as `ASTImporter`, `DiagOpts`, 
and `FrontendAction`, etc. somehow feel better at ~~transmitting~~ reflecting 
upon their meaning in their name.





Comment at: lib/CrossTU/CrossTranslationUnit.cpp:45
+
+/// Recursively visit the funtion decls of a DeclContext, and looks up a
+/// function based on mangled name.

"visit**s** (...) and looks up" or "visit (...) and look up"



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:46
+/// Recursively visit the funtion decls of a DeclContext, and looks up a
+/// function based on mangled name.
+const FunctionDecl *

If we are using //USR// for lookup, then why does this look up "based on 
mangled name"?



Comment at: tools/CMakeLists.txt:25
   add_clang_subdirectory(scan-view)
+  add_clang_subdirectory(clang-func-mapping)
 endif()

The other "blocks" in this file look //somewhat// in alphabetic order, perhaps 
this should be added a few lines above?


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:42
+/// Note that this class also implements caching.
+class CrossTranslationUnit {
+public:

xazax.hun wrote:
> whisperity wrote:
> > Does the name of this class make sense? If I say
> > 
> > 
> > ```
> > CrossTranslationUnit *ctuptr = new CrossTranslationUnit(...);
> > ```
> > 
> > did I construct a new //CrossTranslationUnit//? What even **is** a 
> > //CrossTranslationUnit//? Other class names, such as `ASTImporter`, 
> > `DiagOpts`, and `FrontendAction`, etc. somehow feel better at 
> > ~~transmitting~~ reflecting upon their meaning in their name.
> > 
> > 
> What would you think about `CrossTranslationUnitContext`?
> 
> The functionality of this class is have all the relevant data required for 
> doing Cross TU lookups and also provide the interface for that. 
WFM. I'm not sure whose authority is the class names, but I like it a lot 
better.


https://reviews.llvm.org/D34512



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

[PATCH] D32700: [clang-tidy] Add bugprone-suspicious-memset-usage check.

[Whisperity via Phabricator via cfe-commits]

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

Considering the results published in the opening description:

/home/reka/codechecker_dev_env/llvm/lib/Support/NativeFormatting.cpp:55:29: 

 `warning: memset fill value is char '0', potentially mistaken for int 0 
[bugprone-suspicious-memset-usage]`

  std::memset(NumberBuffer, '0', sizeof(NumberBuffer));
^~~~
0

/home/reka/codechecker_dev_env/llvm/lib/Support/NativeFormatting.cpp:148:26: 

 `warning: memset fill value is char '0', potentially mistaken for int 0 
[bugprone-suspicious-memset-usage]`

  ::memset(NumberBuffer, '0', llvm::array_lengthof(NumberBuffer));
 ^~~~
 0

In these case, the buffer is of `char[]` type. This `memset` call initialises 
the array with the character literal ASCII `0`, which is, in this context, the 
expected behaviour. Maybe you should add an exemption from the checker for 
these cases, i.e. where we **do** know that the buffer is a `char[]`.

Tests should be extended with a case for this addition.


https://reviews.llvm.org/D32700



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

[PATCH] D32700: [clang-tidy] Add bugprone-suspicious-memset-usage check.

[Whisperity via Phabricator via cfe-commits]

whisperity resigned from this revision.
whisperity added inline comments.



Comment at: docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst:10
+
+**Case 1: Fill value is a character '0'**
+

Shouldn't this `'0'` be enclosed within backticks?


https://reviews.llvm.org/D32700



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

[PATCH] D32700: [clang-tidy] Add bugprone-suspicious-memset-usage check.

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp:57-58
+void SuspiciousMemsetUsageCheck::check(const MatchFinder::MatchResult ) 
{
+  // Case 1: fill_char of memset() is a character '0'. Probably an integer zero
+  // was intended.
+  if (const auto *CharZeroFill =

@alexfh Your review on putting the comments within their applicable branch 
bodies applies here too?



Comment at: clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp:89-90
+
+  // Case 3: byte_count of memset() is zero. This is most likely an argument
+  // swap.
+  else if (const auto *Call = Result.Nodes.getNodeAs("call")) {

Same as L57-58.


https://reviews.llvm.org/D32700



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In https://reviews.llvm.org/D34512#800502, @xazax.hun wrote:

> In https://reviews.llvm.org/D34512#800499, @klimek wrote:
>
> > In https://reviews.llvm.org/D34512#800490, @xazax.hun wrote:
> >
> > > It looks like Richard approved libTooling as a dependency for clang on 
> > > the mailing list 
> > > (http://lists.llvm.org/pipermail/cfe-dev/2017-July/054536.html).
> > >  If it is ok to have this code in libTooling (for now), I think we could 
> > > start/continue the review of this patch.
> >
> >
> > I read that somewhat differently? It seems like Richard basically proposes 
> > adding a new library for things that control how we run clang in a multi-TU 
> > scenario. I'd call it libIndex, but that already exists :)
>
>
> No, but since Richard said he did not see any justifications to include this 
> in libTooling, I had the impression this is not his final word, and in case 
> you see it justified, this remains the suggested direction. 
>  But in this case I will rewrite this patch to create a new library. Are you 
> still interested in reviewing it? Do you have any other name in mind? What 
> about libCrossTU?




In https://reviews.llvm.org/D34512#800625, @xazax.hun wrote:

> In https://reviews.llvm.org/D34512#800618, @klimek wrote:
>
> > +Richard as top-level code owner for new libs.
> >
> > For bikeshedding the name: I'd have liked libIndex, but that's already 
> > taken. CrossTU doesn't seem too bad to me, too, though.
>
>
> Some brainstorming for the bikeshedding: libProjet, libProjectIndex, libImport
>  The reason I am not sure about the Index in the name because this code does 
> not contain (yet) an interface to create/handle indexes. 
>  The import might be a good once, since this library is basically wrapping 
> the ASTImporter to import code from external sources, but it might be 
> misleading, since ASTImporter is in the AST module.


Cross-TU is now used (in the lifeline of these features introduced by the team) 
in multiple contexts: CTU analysis, and now CTU lookup.
Maybe the name of this lib should reflect on the fact that the lib is used to 
support **cross-TU definition lookup**.

Hmm... `libCrossTULocator`?


https://reviews.llvm.org/D34512



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



Comment at: lib/AST/ASTContext.cpp:1497
+  auto FnUnitCacheEntry = FunctionAstUnitMap.find(MangledFnName);
+  if (FnUnitCacheEntry == FunctionAstUnitMap.end()) {
+if (FunctionFileMap.empty()) {

danielmarjamaki wrote:
> How about replacing FunctionAstUnitMap.find() with FunctionAstUnitMap.count()?
> 
`FnUnitCacheEntry` is used in line `1525`.


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] D32350: [Analyzer] Exception Checker

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:80
+  bool WasReThrow;
+  Types CurrentThrows; // actually alive exceptions
+  FunctionExceptionsMap

There are some comment formatting issues along these lines.



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:84
+  std::vector CallStack;// trace call hierarchy
+  SmallSet EnabledFuncs;  // allowed functions
+  SmallSet IgnoredExceptions; // ignored exceptions

I had to stop here for a moment and heavily think what this variable (and the 
relevant command-line argument) is used for.

Maybe this calls for a comment then. What is "allowed function"? One that is 
**explicitly** allowed to throw, based on the user's decision? This should be 
explained here.



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:109
+  EnabledFuncs.insert(EnabledFuncsVec.begin(), EnabledFuncsVec.end());
+  EnabledFuncs.insert("swap");
+  EnabledFuncs.insert("main");

Why is `swap` hardcoded as an "enabledfunc"?



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:329
+  const FunctionDecl *callee = C->getDirectCallee();
+  // if is not exist body for this function we do not care about
+  if (!callee || !callee->hasBody()) {

The phrasing should be fixed here for easier understanding.



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:336
+  FunctionExceptionsMap::const_iterator fnIt = ExceptionsThrown.find(name);
+  // already processed?
+  if (fnIt == ExceptionsThrown.end()) {

`already processed` what? A given exception type from a given function?



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:468
+  const auto *D = pNode.get();
+  if (D == nullptr)
+return false;

`if (!D)`



Comment at: test/Analysis/exception-misuse.cpp:18
+
+Y(Y&&) { // expected-warning{{This function should not throw}}
+throw data;

I would use a much more descriptive error message here. E.g., explicitly say, 
that `move (constructor|operator=) should not throw`.



Comment at: test/Analysis/exception-misuse.cpp:26
+
+~Y() {  // expected-warning{{This function should not throw}}
+// nesting

Yet again, better wording: _Destructor not marked `noexcept(false)` should not 
throw_ (this is true since `C++11`, maybe this needs to be based on a 
conditional in the checker!)

@xazax.hun, any idea on what a good error message here should be?



Comment at: test/Analysis/exception-misuse.cpp:26
+
+~Y() {  // expected-warning{{This function should not throw}}
+// nesting

whisperity wrote:
> Yet again, better wording: _Destructor not marked `noexcept(false)` should 
> not throw_ (this is true since `C++11`, maybe this needs to be based on a 
> conditional in the checker!)
> 
> @xazax.hun, any idea on what a good error message here should be?
Also, a test case for a throwing, and `noexcept(false)`-specified dtor is 
missing.


https://reviews.llvm.org/D32350



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


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] 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 ) {
+  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.rst:62

[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] 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] D45095: [clang-tidy] Align usage of ClangTool interface with new VFS injection

[Whisperity via Phabricator via cfe-commits]

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

Update to be in line with contents in dependency patch.


https://reviews.llvm.org/D45095

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp

Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -339,10 +339,8 @@
 
 llvm::IntrusiveRefCntPtr
 getVfsOverlayFromFile(const std::string ) {
-  llvm::IntrusiveRefCntPtr OverlayFS(
-  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
   llvm::ErrorOr Buffer =
-  OverlayFS->getBufferForFile(OverlayFile);
+  vfs::getRealFileSystem()->getBufferForFile(OverlayFile);
   if (!Buffer) {
 llvm::errs() << "Can't load virtual filesystem overlay file '"
  << OverlayFile << "': " << Buffer.getError().message()
@@ -357,8 +355,7 @@
  << OverlayFile << "'.\n";
 return nullptr;
   }
-  OverlayFS->pushOverlay(FS);
-  return OverlayFS;
+  return FS;
 }
 
 static int clangTidyMain(int argc, const char **argv) {
@@ -432,10 +429,10 @@
 llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
 return 1;
   }
-  llvm::IntrusiveRefCntPtr BaseFS(
-  VfsOverlay.empty() ? vfs::getRealFileSystem()
+  llvm::IntrusiveRefCntPtr VirtualFileSystem(
+  VfsOverlay.empty() ? nullptr
  : getVfsOverlayFromFile(VfsOverlay));
-  if (!BaseFS)
+  if (!VfsOverlay.empty() && !VirtualFileSystem)
 return 1;
 
   ProfileData Profile;
@@ -445,8 +442,10 @@
   llvm::InitializeAllAsmParsers();
 
   ClangTidyContext Context(std::move(OwningOptionsProvider));
-  runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
-   EnableCheckProfile ?  : nullptr);
+  ClangTool Tool(OptionsParser.getCompilations(), PathList,
+ std::make_shared(),
+ vfs::createOverlayOnRealFilesystem(VirtualFileSystem));
+  runClangTidy(Context, Tool, EnableCheckProfile ?  : nullptr);
   ArrayRef Errors = Context.getErrors();
   bool FoundErrors =
   std::find_if(Errors.begin(), Errors.end(), [](const ClangTidyError ) {
@@ -459,7 +458,7 @@
 
   // -fix-errors implies -fix.
   handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount,
-   BaseFS);
+   Tool);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
 std::error_code EC;
Index: clang-tidy/tool/CMakeLists.txt
===
--- clang-tidy/tool/CMakeLists.txt
+++ clang-tidy/tool/CMakeLists.txt
@@ -16,6 +16,7 @@
   clangAST
   clangASTMatchers
   clangBasic
+  clangFrontend
   clangTidy
   clangTidyAndroidModule
   clangTidyAbseilModule
Index: clang-tidy/ClangTidy.h
===
--- clang-tidy/ClangTidy.h
+++ clang-tidy/ClangTidy.h
@@ -16,6 +16,7 @@
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
@@ -225,9 +226,7 @@
 /// \param Profile if provided, it enables check profile collection in
 /// MatchFinder, and will contain the result of the profile.
 void runClangTidy(clang::tidy::ClangTidyContext ,
-  const tooling::CompilationDatabase ,
-  ArrayRef InputFiles,
-  llvm::IntrusiveRefCntPtr BaseFS,
+  clang::tooling::ClangTool ,
   ProfileData *Profile = nullptr);
 
 // FIXME: This interface will need to be significantly extended to be useful.
@@ -238,7 +237,7 @@
 /// clang-format configuration file is found, the given \P FormatStyle is used.
 void handleErrors(ClangTidyContext , bool Fix,
   unsigned ,
-  llvm::IntrusiveRefCntPtr BaseFS);
+  clang::tooling::ClangTool );
 
 /// \brief Serializes replacements into YAML and writes them to the specified
 /// output stream.
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -38,7 +38,6 @@
 #include "clang/Tooling/DiagnosticsYaml.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/ReplacementsYaml.h"
-#include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include 
@@ -89,9 +88,9 @@
 
 class ErrorReporter {
 public:
-  ErrorReporter(ClangTidyContext , bool ApplyFixes,
-llvm::IntrusiveRefCntPtr BaseFS)
-  : Files(FileSystemOptions(), BaseFS), DiagOpts(new DiagnosticOptions()),
+  ErrorReporter(ClangTidyContext , bool ApplyFixes, ClangTool )
+  : Files(FileSystemOptions(), Tool.getFiles().getVirtualFileSystem()),
+

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

[Whisperity via Phabricator via cfe-commits]

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

- Use an even more explicit way with the documentation requiring that the file 
system should be an overlay.
- Add a method to easily overlay a `FileSystem` above the real one.


Repository:
  rC Clang

https://reviews.llvm.org/D45094

Files:
  include/clang/Basic/VirtualFileSystem.h
  include/clang/Tooling/Tooling.h
  lib/Basic/VirtualFileSystem.cpp
  lib/Tooling/Tooling.cpp
  unittests/Tooling/ToolingTest.cpp

Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -402,24 +402,39 @@
   EXPECT_FALSE(Found);
 }
 
-TEST(ClangToolTest, BaseVirtualFileSystemUsage) {
+TEST(ClangToolVFSTest, VirtualFileSystemUsage) {
   FixedCompilationDatabase Compilations("/", std::vector());
-  llvm::IntrusiveRefCntPtr OverlayFileSystem(
-  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new vfs::InMemoryFileSystem);
-  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
 
   InMemoryFileSystem->addFile(
-  "a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+  "/a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
 
-  ClangTool Tool(Compilations, std::vector(1, "a.cpp"),
- std::make_shared(), OverlayFileSystem);
+  ClangTool Tool(Compilations, std::vector(1, "/a.cpp"),
+ std::make_shared(),
+ InMemoryFileSystem);
   std::unique_ptr Action(
   newFrontendActionFactory());
   EXPECT_EQ(0, Tool.run(Action.get()));
 }
 
+TEST(ClangToolVFSTest, VFSDoesntContainEveryFile) {
+  FixedCompilationDatabase Compilations("/", std::vector());
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new vfs::InMemoryFileSystem);
+
+  InMemoryFileSystem->addFile(
+  "/a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("#include \n"
+"int main() {}"));
+
+  ClangTool Tool(Compilations, std::vector(1, "/a.cpp"),
+ std::make_shared(),
+ InMemoryFileSystem);
+  std::unique_ptr Action(
+  newFrontendActionFactory());
+  EXPECT_NE(0, Tool.run(Action.get()));
+}
+
 // Check getClangStripDependencyFileAdjuster doesn't strip args after -MD/-MMD.
 TEST(ClangToolTest, StripDependencyFileAdjuster) {
   FixedCompilationDatabase Compilations("/", {"-MD", "-c", "-MMD", "-w"});
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -358,13 +358,13 @@
 ClangTool::ClangTool(const CompilationDatabase ,
  ArrayRef SourcePaths,
  std::shared_ptr PCHContainerOps,
- IntrusiveRefCntPtr BaseFS)
+ IntrusiveRefCntPtr FileSystem)
 : Compilations(Compilations), SourcePaths(SourcePaths),
   PCHContainerOps(std::move(PCHContainerOps)),
-  OverlayFileSystem(new vfs::OverlayFileSystem(std::move(BaseFS))),
-  InMemoryFileSystem(new vfs::InMemoryFileSystem),
-  Files(new FileManager(FileSystemOptions(), OverlayFileSystem)) {
+  InMemoryFileSystem(new vfs::InMemoryFileSystem) {
+  OverlayFileSystem = new vfs::OverlayFileSystem(FileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  Files = new FileManager(FileSystemOptions(), OverlayFileSystem);
   appendArgumentsAdjuster(getClangStripOutputAdjuster());
   appendArgumentsAdjuster(getClangSyntaxOnlyAdjuster());
   appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -325,6 +325,7 @@
 }
 
 void OverlayFileSystem::pushOverlay(IntrusiveRefCntPtr FS) {
+  // FIXME: OverlayFS containing another one in its stack could be flattened.
   FSList.push_back(FS);
   // Synchronize added file systems by duplicating the working directory from
   // the first one in the list.
@@ -366,6 +367,14 @@
   return {};
 }
 
+IntrusiveRefCntPtr
+vfs::createOverlayOnRealFilesystem(IntrusiveRefCntPtr TopFS) {
+  IntrusiveRefCntPtr OverlayFS =
+new OverlayFileSystem(getRealFileSystem());
+  OverlayFS->pushOverlay(TopFS);
+  return OverlayFS;
+}
+
 clang::vfs::detail::DirIterImpl::~DirIterImpl() = default;
 
 namespace {
Index: include/clang/Tooling/Tooling.h
===
--- include/clang/Tooling/Tooling.h
+++ include/clang/Tooling/Tooling.h
@@ -303,14 +303,16 @@
   /// \param SourcePaths The source files to run over. If a source files is
   ///not found in Compilations, it is skipped.
   /// \param PCHContainerOps The PCHContainerOperations for loading and creating
-  /// clang modules.
-  /// \param BaseFS VFS used for all underlying file 

[PATCH] D45096: Allow the creation of human-friendly ASTDumper to arbitrary output stream

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@alexfh I have updated the patch. I don't have commit rights, so if you think 
this is good to go, could you please commit for me?


Repository:
  rC Clang

https://reviews.llvm.org/D45096



___
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 updated this revision to Diff 141206.
whisperity added a comment.

Simplify the patch.


https://reviews.llvm.org/D45094

Files:
  include/clang/Basic/VirtualFileSystem.h
  include/clang/Tooling/Tooling.h
  lib/Basic/VirtualFileSystem.cpp
  lib/Tooling/Tooling.cpp
  unittests/Tooling/ToolingTest.cpp

Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -402,24 +402,39 @@
   EXPECT_FALSE(Found);
 }
 
-TEST(ClangToolTest, BaseVirtualFileSystemUsage) {
+TEST(ClangToolVFSTest, VirtualFileSystemUsage) {
   FixedCompilationDatabase Compilations("/", std::vector());
-  llvm::IntrusiveRefCntPtr OverlayFileSystem(
-  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
   new vfs::InMemoryFileSystem);
-  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
 
   InMemoryFileSystem->addFile(
-  "a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+  "/a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
 
-  ClangTool Tool(Compilations, std::vector(1, "a.cpp"),
- std::make_shared(), OverlayFileSystem);
+  ClangTool Tool(Compilations, std::vector(1, "/a.cpp"),
+ std::make_shared(),
+ InMemoryFileSystem);
   std::unique_ptr Action(
   newFrontendActionFactory());
   EXPECT_EQ(0, Tool.run(Action.get()));
 }
 
+TEST(ClangToolVFSTest, VFSDoesntContainEveryFile) {
+  FixedCompilationDatabase Compilations("/", std::vector());
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new vfs::InMemoryFileSystem);
+
+  InMemoryFileSystem->addFile(
+  "/a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("#include \n"
+"int main() {}"));
+
+  ClangTool Tool(Compilations, std::vector(1, "/a.cpp"),
+ std::make_shared(),
+ InMemoryFileSystem);
+  std::unique_ptr Action(
+  newFrontendActionFactory());
+  EXPECT_NE(0, Tool.run(Action.get()));
+}
+
 // Check getClangStripDependencyFileAdjuster doesn't strip args after -MD/-MMD.
 TEST(ClangToolTest, StripDependencyFileAdjuster) {
   FixedCompilationDatabase Compilations("/", {"-MD", "-c", "-MMD", "-w"});
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -358,10 +358,10 @@
 ClangTool::ClangTool(const CompilationDatabase ,
  ArrayRef SourcePaths,
  std::shared_ptr PCHContainerOps,
- IntrusiveRefCntPtr BaseFS)
+ IntrusiveRefCntPtr FileSystem)
 : Compilations(Compilations), SourcePaths(SourcePaths),
   PCHContainerOps(std::move(PCHContainerOps)),
-  OverlayFileSystem(new vfs::OverlayFileSystem(std::move(BaseFS))),
+  OverlayFileSystem(new vfs::OverlayFileSystem(std::move(FileSystem))),
   InMemoryFileSystem(new vfs::InMemoryFileSystem),
   Files(new FileManager(FileSystemOptions(), OverlayFileSystem)) {
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -325,6 +325,7 @@
 }
 
 void OverlayFileSystem::pushOverlay(IntrusiveRefCntPtr FS) {
+  // FIXME: OverlayFS containing another one in its stack could be flattened.
   FSList.push_back(FS);
   // Synchronize added file systems by duplicating the working directory from
   // the first one in the list.
@@ -366,6 +367,14 @@
   return {};
 }
 
+IntrusiveRefCntPtr
+vfs::createOverlayOnRealFilesystem(IntrusiveRefCntPtr TopFS) {
+  IntrusiveRefCntPtr OverlayFS =
+new OverlayFileSystem(getRealFileSystem());
+  OverlayFS->pushOverlay(TopFS);
+  return OverlayFS;
+}
+
 clang::vfs::detail::DirIterImpl::~DirIterImpl() = default;
 
 namespace {
Index: include/clang/Tooling/Tooling.h
===
--- include/clang/Tooling/Tooling.h
+++ include/clang/Tooling/Tooling.h
@@ -303,14 +303,16 @@
   /// \param SourcePaths The source files to run over. If a source files is
   ///not found in Compilations, it is skipped.
   /// \param PCHContainerOps The PCHContainerOperations for loading and creating
-  /// clang modules.
-  /// \param BaseFS VFS used for all underlying file accesses when running the
-  /// tool.
+  ///clang modules.
+  /// \param FileSystem The Virtual File System that is used to retrieve file
+  ///contents by the tool. In most cases, this should be an overlay upon
+  ///the real file system. (See \p vfs::createOverlayOnRealFilesystem
+  ///for easily creating an overlay.)
   ClangTool(const CompilationDatabase ,
 ArrayRef SourcePaths,
 std::shared_ptr PCHContainerOps =

[PATCH] D45096: Allow the creation of human-friendly ASTDumper to arbitrary output stream

[Whisperity via Phabricator via cfe-commits]

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

Added comments on what `nullptr` means at call sites.


Repository:
  rC Clang

https://reviews.llvm.org/D45096

Files:
  docs/HowToSetupToolingForLLVM.rst
  include/clang/Frontend/ASTConsumers.h
  lib/Frontend/ASTConsumers.cpp
  lib/Frontend/FrontendActions.cpp
  tools/clang-check/ClangCheck.cpp
  tools/clang-import-test/clang-import-test.cpp


Index: tools/clang-import-test/clang-import-test.cpp
===
--- tools/clang-import-test/clang-import-test.cpp
+++ tools/clang-import-test/clang-import-test.cpp
@@ -313,7 +313,8 @@
   auto  = *static_cast(ASTConsumers.back().get());
 
   if (ShouldDumpAST)
-ASTConsumers.push_back(CreateASTDumper("", true, false, false));
+ASTConsumers.push_back(CreateASTDumper(nullptr /*Dump to stdout.*/,
+   "", true, false, false));
 
   CI.getDiagnosticClient().BeginSourceFile(
   CI.getCompilerInstance().getLangOpts(),
Index: tools/clang-check/ClangCheck.cpp
===
--- tools/clang-check/ClangCheck.cpp
+++ tools/clang-check/ClangCheck.cpp
@@ -138,7 +138,9 @@
 if (ASTList)
   return clang::CreateASTDeclNodeLister();
 if (ASTDump)
-  return clang::CreateASTDumper(ASTDumpFilter, /*DumpDecls=*/true,
+  return clang::CreateASTDumper(nullptr /*Dump to stdout.*/,
+ASTDumpFilter,
+/*DumpDecls=*/true,
 /*Deserialize=*/false,
 /*DumpLookups=*/false);
 if (ASTPrint)
Index: lib/Frontend/FrontendActions.cpp
===
--- lib/Frontend/FrontendActions.cpp
+++ lib/Frontend/FrontendActions.cpp
@@ -74,7 +74,8 @@
 
 std::unique_ptr
 ASTDumpAction::CreateASTConsumer(CompilerInstance , StringRef InFile) {
-  return CreateASTDumper(CI.getFrontendOpts().ASTDumpFilter,
+  return CreateASTDumper(nullptr /*Dump to stdout.*/,
+ CI.getFrontendOpts().ASTDumpFilter,
  CI.getFrontendOpts().ASTDumpDecls,
  CI.getFrontendOpts().ASTDumpAll,
  CI.getFrontendOpts().ASTDumpLookups);
Index: lib/Frontend/ASTConsumers.cpp
===
--- lib/Frontend/ASTConsumers.cpp
+++ lib/Frontend/ASTConsumers.cpp
@@ -138,12 +138,14 @@
FilterString);
 }
 
-std::unique_ptr clang::CreateASTDumper(StringRef FilterString,
-bool DumpDecls,
-bool Deserialize,
-bool DumpLookups) {
+std::unique_ptr
+clang::CreateASTDumper(std::unique_ptr Out,
+   StringRef FilterString,
+   bool DumpDecls,
+   bool Deserialize,
+   bool DumpLookups) {
   assert((DumpDecls || Deserialize || DumpLookups) && "nothing to dump");
-  return llvm::make_unique(nullptr,
+  return llvm::make_unique(std::move(Out),
Deserialize ? ASTPrinter::DumpFull :
DumpDecls ? ASTPrinter::Dump :
ASTPrinter::None,
Index: include/clang/Frontend/ASTConsumers.h
===
--- include/clang/Frontend/ASTConsumers.h
+++ include/clang/Frontend/ASTConsumers.h
@@ -34,9 +34,10 @@
 std::unique_ptr CreateASTPrinter(std::unique_ptr OS,
   StringRef FilterString);
 
-// AST dumper: dumps the raw AST in human-readable form to stderr; this is
-// intended for debugging.
-std::unique_ptr CreateASTDumper(StringRef FilterString,
+// AST dumper: dumps the raw AST in human-readable form to the given output
+// stream, or stdout if OS is nullptr.
+std::unique_ptr CreateASTDumper(std::unique_ptr OS,
+ StringRef FilterString,
  bool DumpDecls, bool Deserialize,
  bool DumpLookups);
 
Index: docs/HowToSetupToolingForLLVM.rst
===
--- docs/HowToSetupToolingForLLVM.rst
+++ docs/HowToSetupToolingForLLVM.rst
@@ -133,7 +133,8 @@
   if (this->ASTList.operator _Bool())
   return clang::CreateASTDeclNodeLister();
   if (this->ASTDump.operator _Bool())
-  return clang::CreateASTDumper(this->ASTDumpFilter);
+  return clang::CreateASTDumper(nullptr /*Dump to stdout.*/,
+this->ASTDumpFilter);
   if (this->ASTPrint.operator _Bool())
   

[PATCH] D45096: Allow the creation of human-friendly ASTDumper to arbitrary output stream

[Whisperity via Phabricator via cfe-commits]

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

It is also std-out (`llvm::outs()`) in case of `nullptr` and not std-err.


Repository:
  rC Clang

https://reviews.llvm.org/D45096



___
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] 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] 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/ClangTidy.cpp:91
 public:
-  ErrorReporter(ClangTidyContext , bool ApplyFixes,
-llvm::IntrusiveRefCntPtr BaseFS)
-  : Files(FileSystemOptions(), BaseFS), DiagOpts(new DiagnosticOptions()),
+  ErrorReporter(ClangTidyContext , bool ApplyFixes, ClangTool )
+  : Files(FileSystemOptions(), Tool.getFiles().getVirtualFileSystem()),

ilya-biryukov wrote:
> Why do we need to change the signature of `ErrorReporter` constructor?
> Broadening the accepted interface does not seem right. It only needs the vfs 
> and the clients could get vfs from `ClangTool` themselves.
Is it okay interface-wise if the FS used by the `ErrorReporter` is **not** the 
same as the one used by the module that produced the errors? It seems like an 
undocumented consensus/convention that the same VFS should have been passed 
here.



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

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.


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] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: include/clang/Basic/VirtualFileSystem.h:315
+IntrusiveRefCntPtr
+createOverlayOnRealFilesystem(IntrusiveRefCntPtr TopFS);
+

ilya-biryukov wrote:
> NIT: I'm not an expert in English, but shouldn't it be 
> createOverlay**Over**Real.
> Also maybe shorten the suffix: `createOverlayOverRealFS`?
According to [[https://www.thefreedictionary.com/overlay|this dictionary]] 
overlay can mean "to lay //on// " something. Although `above` could also work 
to eliminate saying "over" repeatedly.



Comment at: unittests/Tooling/ToolingTest.cpp:411
   InMemoryFileSystem->addFile(
-  "a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+  "/a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
 

ilya-biryukov wrote:
> Using `'/'` in paths would probably break on Windows.
> Why do we need to add it in the first place?
Agreed, this will be removed. It was added because overlaying the real-FS 
changes the working directory in the memory filesystem too, which in the 
previous patch (where the overlay was done by the ClangTool constructor) broke 
where the file resides. It's not applicable anymore.

Although it's strange that saying `make check-clang-tooling` did **NOT** 
execute these tests and said everything passed, I had to run the build 
`ToolingTest` binary manually!



Comment at: unittests/Tooling/ToolingTest.cpp:435
+  newFrontendActionFactory());
+  EXPECT_NE(0, Tool.run(Action.get()));
+}

ilya-biryukov wrote:
> It's not clear what this tests expects when looking at the code
> A comment would be helpful.
> 
> Also consider matching on an actual error diagnostic (i.e. " not 
> found", right?)
How can I match on the error message?


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] D45096: Allow the creation of human-friendly ASTDumper to arbitrary output stream

[Whisperity via Phabricator via cfe-commits]

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

- Overload removed, now only one `CreateASTDumper` function remains.
- Updated the call sites of this function to use this call.


Repository:
  rC Clang

https://reviews.llvm.org/D45096

Files:
  docs/HowToSetupToolingForLLVM.rst
  include/clang/Frontend/ASTConsumers.h
  lib/Frontend/ASTConsumers.cpp
  lib/Frontend/FrontendActions.cpp
  tools/clang-check/ClangCheck.cpp
  tools/clang-import-test/clang-import-test.cpp


Index: tools/clang-import-test/clang-import-test.cpp
===
--- tools/clang-import-test/clang-import-test.cpp
+++ tools/clang-import-test/clang-import-test.cpp
@@ -313,7 +313,7 @@
   auto  = *static_cast(ASTConsumers.back().get());
 
   if (ShouldDumpAST)
-ASTConsumers.push_back(CreateASTDumper("", true, false, false));
+ASTConsumers.push_back(CreateASTDumper(nullptr, "", true, false, false));
 
   CI.getDiagnosticClient().BeginSourceFile(
   CI.getCompilerInstance().getLangOpts(),
Index: tools/clang-check/ClangCheck.cpp
===
--- tools/clang-check/ClangCheck.cpp
+++ tools/clang-check/ClangCheck.cpp
@@ -138,7 +138,8 @@
 if (ASTList)
   return clang::CreateASTDeclNodeLister();
 if (ASTDump)
-  return clang::CreateASTDumper(ASTDumpFilter, /*DumpDecls=*/true,
+  return clang::CreateASTDumper(nullptr, ASTDumpFilter,
+/*DumpDecls=*/true,
 /*Deserialize=*/false,
 /*DumpLookups=*/false);
 if (ASTPrint)
Index: lib/Frontend/FrontendActions.cpp
===
--- lib/Frontend/FrontendActions.cpp
+++ lib/Frontend/FrontendActions.cpp
@@ -74,7 +74,8 @@
 
 std::unique_ptr
 ASTDumpAction::CreateASTConsumer(CompilerInstance , StringRef InFile) {
-  return CreateASTDumper(CI.getFrontendOpts().ASTDumpFilter,
+  return CreateASTDumper(nullptr,
+ CI.getFrontendOpts().ASTDumpFilter,
  CI.getFrontendOpts().ASTDumpDecls,
  CI.getFrontendOpts().ASTDumpAll,
  CI.getFrontendOpts().ASTDumpLookups);
Index: lib/Frontend/ASTConsumers.cpp
===
--- lib/Frontend/ASTConsumers.cpp
+++ lib/Frontend/ASTConsumers.cpp
@@ -138,12 +138,14 @@
FilterString);
 }
 
-std::unique_ptr clang::CreateASTDumper(StringRef FilterString,
-bool DumpDecls,
-bool Deserialize,
-bool DumpLookups) {
+std::unique_ptr
+clang::CreateASTDumper(std::unique_ptr Out,
+   StringRef FilterString,
+   bool DumpDecls,
+   bool Deserialize,
+   bool DumpLookups) {
   assert((DumpDecls || Deserialize || DumpLookups) && "nothing to dump");
-  return llvm::make_unique(nullptr,
+  return llvm::make_unique(std::move(Out),
Deserialize ? ASTPrinter::DumpFull :
DumpDecls ? ASTPrinter::Dump :
ASTPrinter::None,
Index: include/clang/Frontend/ASTConsumers.h
===
--- include/clang/Frontend/ASTConsumers.h
+++ include/clang/Frontend/ASTConsumers.h
@@ -34,9 +34,10 @@
 std::unique_ptr CreateASTPrinter(std::unique_ptr OS,
   StringRef FilterString);
 
-// AST dumper: dumps the raw AST in human-readable form to stderr; this is
-// intended for debugging.
-std::unique_ptr CreateASTDumper(StringRef FilterString,
+// AST dumper: dumps the raw AST in human-readable form to the given output
+// stream.
+std::unique_ptr CreateASTDumper(std::unique_ptr OS,
+ StringRef FilterString,
  bool DumpDecls, bool Deserialize,
  bool DumpLookups);
 
Index: docs/HowToSetupToolingForLLVM.rst
===
--- docs/HowToSetupToolingForLLVM.rst
+++ docs/HowToSetupToolingForLLVM.rst
@@ -133,7 +133,7 @@
   if (this->ASTList.operator _Bool())
   return clang::CreateASTDeclNodeLister();
   if (this->ASTDump.operator _Bool())
-  return clang::CreateASTDumper(this->ASTDumpFilter);
+  return clang::CreateASTDumper(nullptr, this->ASTDumpFilter);
   if (this->ASTPrint.operator _Bool())
   return clang::CreateASTPrinter(::outs(), this->ASTDumpFilter);
   return new clang::ASTConsumer();


Index: tools/clang-import-test/clang-import-test.cpp

[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] 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] 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] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

[Whisperity via Phabricator via cfe-commits]

whisperity created this revision.
whisperity added reviewers: alexfh, klimek.
whisperity added a project: clang.
Herald added subscribers: dkrupp, rnkovacs.

This patch extends upon https://reviews.llvm.org/D41947 because the interface 
that was landed from that patch isn't much user-friendly.

Injecting a custom virtual file system into the tool is a dangerous operation. 
If the real file system (where the installed Clang's headers reside) are not 
mirrored, it only turns out at `ClangTool::run()` that something was not 
mounted properly. Originally, the execution of a `ClangTool` always used the 
real filesystem.

Starting with https://reviews.llvm.org/D41947, the client code setting the tool 
up must manually create the OverlayFS  (which is, in turn, added as an 
OverlayFS into the tool' inner OverlayFS) containing the real system and the 
user's intention. I believe this logic should not be duplicated in client code, 
and the more traditional use-case of overlaying the filesystem view with some 
files, but by default keeping the real deal beneath it must be reflected better 
on the interface. Client code can now much more **explicitly** specify the 
complete cessation of real filesystem usage.


Repository:
  rC Clang

https://reviews.llvm.org/D45094

Files:
  include/clang/Tooling/Tooling.h
  lib/Tooling/Tooling.cpp
  unittests/Tooling/ToolingTest.cpp

Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -402,24 +402,48 @@
   EXPECT_FALSE(Found);
 }
 
-TEST(ClangToolTest, BaseVirtualFileSystemUsage) {
+TEST(ClangToolVFSTest, MustGiveFileSystem) {
+  FixedCompilationDatabase Compilations("/", std::vector());
+
+  EXPECT_DEATH(ClangTool(Compilations, std::vector(),
+ std::make_shared(),
+ nullptr, false),
+   "without providing a FileSystem");
+}
+
+TEST(ClangToolVFSTest, VirtualFileSystemUsage) {
   FixedCompilationDatabase Compilations("/", std::vector());
-  llvm::IntrusiveRefCntPtr OverlayFileSystem(
-  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
   llvm::IntrusiveRefCntPtr InMemoryFileSystem(
-  new vfs::InMemoryFileSystem);
-  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+new vfs::InMemoryFileSystem);
 
   InMemoryFileSystem->addFile(
-  "a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+"/a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
 
-  ClangTool Tool(Compilations, std::vector(1, "a.cpp"),
- std::make_shared(), OverlayFileSystem);
+  ClangTool Tool(Compilations, std::vector(1, "/a.cpp"),
+ std::make_shared(),
+ InMemoryFileSystem, false);
   std::unique_ptr Action(
-  newFrontendActionFactory());
+newFrontendActionFactory());
   EXPECT_EQ(0, Tool.run(Action.get()));
 }
 
+TEST(ClangToolVFSTest, VFSDoesntContainEveryFile) {
+  FixedCompilationDatabase Compilations("/", std::vector());
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+new vfs::InMemoryFileSystem);
+
+  InMemoryFileSystem->addFile(
+"/a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("#include \n"
+   "int main() {}"));
+
+  ClangTool Tool(Compilations, std::vector(1, "/a.cpp"),
+ std::make_shared(),
+ InMemoryFileSystem, false);
+  std::unique_ptr Action(
+newFrontendActionFactory());
+  EXPECT_NE(0, Tool.run(Action.get()));
+}
+
 // Check getClangStripDependencyFileAdjuster doesn't strip args after -MD/-MMD.
 TEST(ClangToolTest, StripDependencyFileAdjuster) {
   FixedCompilationDatabase Compilations("/", {"-MD", "-c", "-MMD", "-w"});
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -358,13 +358,22 @@
 ClangTool::ClangTool(const CompilationDatabase ,
  ArrayRef SourcePaths,
  std::shared_ptr PCHContainerOps,
- IntrusiveRefCntPtr BaseFS)
+ IntrusiveRefCntPtr FileSystem,
+ bool AlsoUseRealFileSystem)
 : Compilations(Compilations), SourcePaths(SourcePaths),
   PCHContainerOps(std::move(PCHContainerOps)),
-  OverlayFileSystem(new vfs::OverlayFileSystem(std::move(BaseFS))),
-  InMemoryFileSystem(new vfs::InMemoryFileSystem),
-  Files(new FileManager(FileSystemOptions(), OverlayFileSystem)) {
+  InMemoryFileSystem(new vfs::InMemoryFileSystem) {
+  assert(!(!AlsoUseRealFileSystem && !FileSystem) && "ClangTool initialized "
+ "without providing a FileSystem to get files from!");
+  if (AlsoUseRealFileSystem) {
+OverlayFileSystem = new vfs::OverlayFileSystem(vfs::getRealFileSystem());
+if (FileSystem)
+  OverlayFileSystem->pushOverlay(FileSystem);
+  }
+  else
+OverlayFileSystem = new 

[PATCH] D45096: Allow the creation of human-friendly ASTDumper to arbitrary output stream

[Whisperity via Phabricator via cfe-commits]

whisperity created this revision.
whisperity added reviewers: alexfh, klimek, rsmith.
whisperity added a project: clang.
Herald added subscribers: dkrupp, rnkovacs.

`ASTPrinter` allows setting the ouput to any O-Stream, but that printer creates 
source-code-like syntax (and is also marked with a `FIXME`). The nice, 
colourful, mostly human-readable `ASTDumper` only works on the standard error, 
which is not feasible in case a user wants to see the AST of a file through a 
code navigation/comprehension tool.

This small addition of an overload solves generating a nice colourful AST block 
for the users of a tool I'm working on, CodeCompass 
, as opposed to having to duplicate the 
behaviour of definitions that only exist in the anonymous namespace of 
implementation TUs related to this module.


Repository:
  rC Clang

https://reviews.llvm.org/D45096

Files:
  include/clang/Frontend/ASTConsumers.h
  lib/Frontend/ASTConsumers.cpp


Index: lib/Frontend/ASTConsumers.cpp
===
--- lib/Frontend/ASTConsumers.cpp
+++ lib/Frontend/ASTConsumers.cpp
@@ -142,8 +142,18 @@
 bool DumpDecls,
 bool Deserialize,
 bool DumpLookups) {
+  return CreateASTDumper(nullptr, FilterString,
+ DumpDecls, Deserialize, DumpLookups);
+}
+
+std::unique_ptr
+clang::CreateASTDumper(std::unique_ptr Out,
+   StringRef FilterString,
+   bool DumpDecls,
+   bool Deserialize,
+   bool DumpLookups) {
   assert((DumpDecls || Deserialize || DumpLookups) && "nothing to dump");
-  return llvm::make_unique(nullptr,
+  return llvm::make_unique(std::move(Out),
Deserialize ? ASTPrinter::DumpFull :
DumpDecls ? ASTPrinter::Dump :
ASTPrinter::None,
Index: include/clang/Frontend/ASTConsumers.h
===
--- include/clang/Frontend/ASTConsumers.h
+++ include/clang/Frontend/ASTConsumers.h
@@ -40,6 +40,13 @@
  bool DumpDecls, bool Deserialize,
  bool DumpLookups);
 
+// AST dumper: dumps the raw AST in human-readable form to the given output
+// stream.
+std::unique_ptr CreateASTDumper(std::unique_ptr OS,
+ StringRef FilterString,
+ bool DumpDecls, bool Deserialize,
+ bool DumpLookups);
+
 // AST Decl node lister: prints qualified names of all filterable AST Decl
 // nodes.
 std::unique_ptr CreateASTDeclNodeLister();


Index: lib/Frontend/ASTConsumers.cpp
===
--- lib/Frontend/ASTConsumers.cpp
+++ lib/Frontend/ASTConsumers.cpp
@@ -142,8 +142,18 @@
 bool DumpDecls,
 bool Deserialize,
 bool DumpLookups) {
+  return CreateASTDumper(nullptr, FilterString,
+ DumpDecls, Deserialize, DumpLookups);
+}
+
+std::unique_ptr
+clang::CreateASTDumper(std::unique_ptr Out,
+   StringRef FilterString,
+   bool DumpDecls,
+   bool Deserialize,
+   bool DumpLookups) {
   assert((DumpDecls || Deserialize || DumpLookups) && "nothing to dump");
-  return llvm::make_unique(nullptr,
+  return llvm::make_unique(std::move(Out),
Deserialize ? ASTPrinter::DumpFull :
DumpDecls ? ASTPrinter::Dump :
ASTPrinter::None,
Index: include/clang/Frontend/ASTConsumers.h
===
--- include/clang/Frontend/ASTConsumers.h
+++ include/clang/Frontend/ASTConsumers.h
@@ -40,6 +40,13 @@
  bool DumpDecls, bool Deserialize,
  bool DumpLookups);
 
+// AST dumper: dumps the raw AST in human-readable form to the given output
+// stream.
+std::unique_ptr CreateASTDumper(std::unique_ptr OS,
+ StringRef FilterString,
+ bool DumpDecls, bool Deserialize,
+ bool DumpLookups);
+
 // AST Decl node lister: prints qualified names of all filterable AST Decl
 // nodes.
 std::unique_ptr CreateASTDeclNodeLister();
___
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] 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 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] 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 :
-

[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] 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] 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()) {
 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  = C.getSymbolManager();
-  auto  = 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 ,
+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  = 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] 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] 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 , 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/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] 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 

[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] 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] 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 ,
+  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.second 

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

[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] 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] 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] 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] 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] 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] 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] 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: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] 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] 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] 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 )` 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] 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] 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] 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] 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] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message

[Whisperity via Phabricator via cfe-commits]

whisperity accepted this revision.
whisperity added a comment.

@Szelethus Nope.


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] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:81-82
+  diag(PtrArith->getBeginLoc(),
+   "pointer arithmetic is applied to the result of %0() instead of its "
+   "argument")
+  << Func->getName() << Hint;

baloghadamsoftware wrote:
> whisperity wrote:
> > If I put the `+ b` on `X` as in `malloc(X + b)` instead of `malloc(X) + b`, 
> > then it's not //pointer// arithmetic anymore, but (hopefully unsigned) 
> > arithmetic. Should the warning message really start with "pointer 
> > arithmetic"?
> > 
> > Maybe you could consider the check saying
> > 
> > arithmetic operation applied to pointer result of ...() instead of 
> > size-like argument
> > 
> > optionally, I'd clarify it further by putting at the end:
> > 
> > resulting in ignoring a prefix of the buffer.
> > 
> > considering you specifically match on the std(-like) allocations. (At least 
> > for now.)
> "resulting in ignoring a prefix of the buffer" <- this is only true for 
> addition. What should we write for subtraction?
You are right about subtraction. I think this message is concise as is.


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

https://reviews.llvm.org/D71001



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

[PATCH] D71199: [clang-tidy] New check readability-prefer-initialization-list

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Can you refresh my memory on whether a rule for "if init expr is constant, 
initialise in class body instead" exists for init list members? If so, this 
will be a funny "two pass needed to fix" kind of check.




Comment at: 
clang-tools-extra/clang-tidy/readability/PreferInitializationListCheck.cpp:19
+
+static bool isControlStatement(const Stmt *S) {
+  return isa(S) ||

I'm working on a checker which has the need for similarly knowing occurrences 
of "control flow breaking statements". How about `goto` and calling a 
`[[noreturn]]` function, such as (`std::`)`longjmp`? Or there is no point in 
matching such in your checker?



Comment at: 
clang-tools-extra/clang-tidy/readability/PreferInitializationListCheck.cpp:71
+const MatchFinder::MatchResult ) {
+  // FIXME: Add callback implementation.
+  const auto *Ctor = Result.Nodes.getNodeAs("ctor");

FIXME remained. Did you upload the right patch set?



Comment at: 
clang-tools-extra/clang-tidy/readability/PreferInitializationListCheck.cpp:85
+if (const NamedDecl* Mbr = isAssignmentToMemberOf(Class, S)) {
+  diag(S->getBeginLoc(), "%0 can be initialized in the initializer list"
+   " of the constructor") << Mbr->getName();

can -> should?



Comment at: 
clang-tools-extra/clang-tidy/readability/PreferInitializationListCheck.h:18
+
+/// FIXME: Write a short description.
+///

FIXME remained here.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-prefer-initialization-list.rst:7
+Finds member initializations in the constructor body which can be placed into
+the initialization list instead. This does not only improves the readability of
+the code but also affects positively its performance. Class-member assignments

improves -> improve



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-prefer-initialization-list.rst:8
+the initialization list instead. This does not only improves the readability of
+the code but also affects positively its performance. Class-member assignments
+inside a control statement or following the first control statement are 
ignored.

word order: also positively affects



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-prefer-initialization-list.rst:27
+
+Here ``n`` can be initialized in the construcotr list, but ``m`` not, because
+its initialization follow a control statement (``if``):

typo: construcotr



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-prefer-initialization-list.rst:27-28
+
+Here ``n`` can be initialized in the construcotr list, but ``m`` not, because
+its initialization follow a control statement (``if``):
+

whisperity wrote:
> typo: construcotr
[l]ist, unlike `m`, as `m`'s initialization follow a control statement (`if`)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71199



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

[PATCH] D71199: [clang-tidy] New check readability-prefer-initialization-list

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-prefer-initialization-list.cpp:221
+m = 1;
+// NO-MESSAGES: initialization of m follows a conditional expression
+  }

Comment diverged from what's actually written in the code.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71199



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

[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Are buffers otherwise modelled by the Analyser, such as results of the `malloc` 
family and `alloca` intentionally not matched, or are there some tests missing 
regarding this?




Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:115-117
+  llvm::formatv("Argument of default placement new provides storage "
+"capacity of {0} bytes, but the allocated type "
+"requires storage capacity of {1} bytes",

This message might be repeating phrases too much, and seems long. Also, I would 
expect things like //default placement new// or //argument of placement new// 
to be confusing. Not every person running Clang SA knows the nitty-gritty of 
the standard by heart...

More nitpicking: even the "default" (what does this even mean, again?) 
placement new takes **two** arguments, albeit written in a weird grammar, so 
there is no "argument of" by the looks of it. I really think this is confusing.

Something more concise, simpler, still getting the meaning across:

> Storage provided to placement new is only `N` bytes, whereas allocating a `T` 
> requires `M` bytes



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612



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

[PATCH] D20689: [clang-tidy] Suspicious Call Argument checker

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

I have developed a related check in D69560 . 
That one considers types, but is an //interface rule// checker, and does not 
consider (any) potential call sites. Moreover, it does not consider "swaps" 
that happen across a function call, only, as the name implies, //adjacent// 
similar-type ranges.

Maybe one could lift the "is-similar-type", or rather, 
"is-accidentally-mixable-type" related ruling to some common location, and use 
type similarity as a precondition gate in the reports of this check?




Comment at: docs/clang-tidy/checks/misc-redundant-expression.rst:18
 
-Example:
+Examples:
 

This seems to be an unrelated diff.



Comment at: test/clang-tidy/misc-redundant-expression.cpp:20
   if (X - X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are 
equivalent [misc-redundant-expression]
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are 
equivalent [misc-redundant-expression]
   if (X / X) return 1;

This entire file seems to be unrelated to the discussion at hand, perhaps a 
rebase went sideways?


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] D71001: [clang-tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

While I can't pitch in with actual findings of this check, @MyDeveloperDay, 
you're right in many aspects, including those specific examples //not// firing. 
But an example that actually fires this check indicates a very specific 
**undefined behaviour** case. So if such bogus code (that would trigger on this 
check) did not cause run-time issues so far, it's because they never `free()` 
their memory allocations (really bad?), or their platform is particular enough 
that it allows calling `free` into the buffer, not only for the **start** of 
the buffer.

You must (only at most once) free each and every pointer as they had returned 
from a function of the `*alloc` family. `free(malloc(X))` is okay, but 
`free(malloc(X) + b)` is, as per the rules of the language, clearly undefined 
behaviour. Which of course may just as well include the OS/runtime going "Oh 
the stupid `free`d into this buffer, let me find the beginning of the 
allocation..." until one day it doesn't anymore.

(Snarky "self"-advertisement: Did you check CodeChecker to be that bug 
database?  )




Comment at: 
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:23
+  anyOf(hasName("::malloc"), hasName("std::malloc"), hasName("::alloca"),
+hasName("std::alloca"), hasName("::calloc"), 
hasName("std::calloc"),
+hasName("::realloc"), hasName("std::realloc")));

`alloca` is a linux/posix-specific thing, and as such, I don't think it 
//could// be part of the `std` namespace.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp:81-82
+  diag(PtrArith->getBeginLoc(),
+   "pointer arithmetic is applied to the result of %0() instead of its "
+   "argument")
+  << Func->getName() << Hint;

If I put the `+ b` on `X` as in `malloc(X + b)` instead of `malloc(X) + b`, 
then it's not //pointer// arithmetic anymore, but (hopefully unsigned) 
arithmetic. Should the warning message really start with "pointer arithmetic"?

Maybe you could consider the check saying

arithmetic operation applied to pointer result of ...() instead of 
size-like argument

optionally, I'd clarify it further by putting at the end:

resulting in ignoring a prefix of the buffer.

considering you specifically match on the std(-like) allocations. (At least for 
now.)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D71001



___
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 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] 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 , 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 ) {}
+// 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 , const T ) {}
+// 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 , const T t2) {}
+//