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

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

D50353 has landed, so after a rebase this patch will not compile.

In D50488#1225064, @george.karpenkov wrote:

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

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.

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

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

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

Generally grammar / phrasing things that have caught my eye.

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.

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.

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

Akin to D45094, pinging this too. 🙂

Pinging this as the talk has stalled.

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

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

Two minor comments.

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(?):

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

There is something that came up in my mind:

In D45532#1068700, @Szelethus wrote:

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

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

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

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

Added comments on what nullptr means at call sites.

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

Update to be in line with contents in dependency patch.

Simplify the patch.

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

@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?

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

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

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?

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 D30691, but I have seen some asserts that "imported function should already have a body" and such.

Considering the results published in the opening description:

In D34512#800502, @xazax.hun wrote:

In D34512#800499, @klimek wrote:

In 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?