That's a quite impressive amount of work. Thanks! Looks good.

LG, thanks!

LG. Thank you!

In rL359942#647163, @grimar wrote:

Perhaps even better would be to not create output at all. I.e -o /dev/null.

Apart from NOLINT handling there's more logic in ClangTidyDiagnosticConsumer::HandleDiagnostic, which isn't properly transferred to ClangTidyContext::diag in this patch. The logic that is transferred seems to change the behavior w.r.t. notes that can "unmute" ignored warnings (see https://reviews.llvm.org/D59135#1456108). I suspect that we're missing proper test coverage here. Another issue is that compiler diagnostics don't pass ClangTidyContext::diag in the non-plugin use case. Do all the existing tests pass with your patch?

Please regenerate the HTML docs using clang/docs/tools/dump_ast_matchers.py.

LG. Thanks for the fix!

LG

In D60670#1474333, @Charusso wrote:

In D60670#1470849, @alexfh wrote:

Can we add an option to enable or disable these notes similar to Clang's -fdiagnostics-show-option, -fdiagnostics-show-note-include-stack, etc?

@alexfh, add them as a Clang option with -f? I do not know how Clang Tidy would use that, so that I am not sure. I would create an AnalyzerOptions option as I see that far only.

LG with a comment.

There's one more nit. Otherwise good to go.

LG with a couple of nits.

In D60670#1469494, @NoQ wrote:

Let's decide whether we want to display the new notes in the text output. @alexfh: Given that clang-tidy is the primary consumer of the analyzer's text output mode, would you rather have or rather not have these new note:s in the text output? (scroll to the bottom of D53076 to see how they look). In my opinion, i'd rather have them in text output (because text output is hard to navigate, and having the necessary information exactly where you need it would probably be helpful), but rather not have them in tools that parse clang-tidy notes and stuff them into a source code view in some GUI (such tools do exist, right?), so im confused. I'd also prefer to have them be off in text output in tests, because such tests usually test checker-specific notes and having more checker-inspecific notes in the tests is more annoying than useful. But that's pretty minor; i'll be happy to write them down every time i write a test if they are believed to be a good thing in text output in general.

Awesome, thanks!
LG

A few post-commit comments.

Artem, I'd appreciate, if you found time to finish this fix. The bug is causing most failures in our setup. Thanks!

Thanks! The change looks good now.

Thanks for the useful check! I have a few comments, see inline.

In D59135#1458265, @thorsten-klein wrote:

Hello @alexfh ,
Let me extend your example

$ cat a.cc 
#include "b.h"
#include "d.h"
int main(){check(nullptr);}
$ cat b.h 
#include "c.h"
inline void b() { c(/*y=*/42); }
$ cat c.h 
void c(int x);
$ cat d.h 
inline char* check(char* buffer)
{
	*buffer++=1; // Should be clang-analyzer-core.NullDereference
	return buffer;
}

Now an additional warning is found and shown (=not suppressed):

$ clang-tidy -checks=-*,clang-*,bugprone-argument-comment a.cc --
2 warnings generated.
/home/default/Temp/clang-tidy-test/d.h:3:11: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^
/home/default/Temp/clang-tidy-test/a.cc:3:12: note: Calling 'check'
int main(){check(0);}
           ^
/home/default/Temp/clang-tidy-test/d.h:3:3: note: Null pointer value stored to 'buffer'
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
         ^
/home/default/Temp/clang-tidy-test/d.h:3:11: note: Dereference of null pointer
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

It looks like there's a number of users of this function beyond what you've mentioned:
clang-tidy/readability/IsolateDeclarationCheck.cpp:210
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:43
clang-tidy/readability/NamespaceCommentCheck.cpp:106
clang/lib/ARCMigrate/PlistReporter.cpp:110
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:192

Looks like this check would fit better into the bugprone module.

Can you give a specific example of how this problem manifests?

In D59540#1456011, @MyDeveloperDay wrote:

friendly ping

Thanks, looks better now, but still a few comments, mostly nits.

In D60151#1454105, @hintonda wrote:

• Rename llvm directory to llvm_project.
• Change llvm- to llvm-project-.
• Rename files.

Thanks for addressing this! Please add the test cases from https://bugs.llvm.org/show_bug.cgi?id=41185

This looks like a more promising direction. Thanks for the readiness to experiment with this.

In D59449#1449664, @zinovy.nis wrote:

Why not just use clang-tidy-diff.py? The clang-tidy-diff.py script has a distinct and somewhat self-documenting name and a very specific purpose (find clang-tidy regressions in a patch), while run-clang-tidy.py is more focused on running over larger bodies of code with a purpose of analyzing or cleaning up. Is there any benefit in having all functionality in run-clang-tidy.py?

Both scripts have almost the same implementation, almost the same syntax (at least after -j and -timeout we introduced). So why not merge them.

Can you verify that the add_new_check.py script works fine with this new module?
Thanks!

The change looks fine, but I don't understand the description of this revision. Could you clarify which checkers you're talking about and which bug you observe?

In D59859#1444333, @aaron.ballman wrote:

In D59859#1444176, @lebedev.ri wrote:

I'm not sure why we want this? What is wrong with simply applying clang-tidy twice?

It doubles the execution time of checking a large project (which may be unacceptably slow), and there's no guarantee that twice will be enough in the general case (one set of fixes may trigger a different check's diagnostics, which may get fixed and trigger another check's diagnostics, etc).

In D59449#1435836, @zinovy.nis wrote:

In D59449#1435032, @alexfh wrote:

How is this functionality different from the clang-tidy-diff.py script with the -j option being added in the other patch?

It's the same.

Looks reasonable in general, but we usually add modules with at least one check. Let's do the same here.

I've posted another test case to https://bugs.llvm.org/show_bug.cgi?id=41239. It may already be covered by the fix. Could you check?

LG

LG. This fixes (at least, most of) the crashes we observe.