LG with an outstanding comment.

In D72527#1817518, @Eugene.Zelenko wrote:

Both scripts works fine. However rename script should also sort entries alphabetically, but probably this should be separate patch.

LG with a nit, if this works.

Awesome! Thanks for updating the script! A few nits, otherwise LG, if this works.

What version of clang-tidy and clang-format does this run? Whatever is available in Debian packages?

This list used to be automatically updated by the clang-tools-extra/clang-tidy/add_new_check.py script. I suppose, this is broken now?

• Added a test with an empty attribute list.

In D70974#1769036, @gribozavr2 wrote:

In D70974#1768902, @aaron.ballman wrote:

In D70974#1768871, @gribozavr2 wrote:

I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias?

While I think that's a superior solution to using macros, some users have macros instead. This fixes a bug reported in https://bugs.llvm.org/show_bug.cgi?id=26274 and I agree that the behavior described in that bug is not what I would expect it to be.

I mean, it is possible to break pretty much any ClangTidy check with sneaky code. But there's a limit to which we should try to make things work in tricky corner cases. For example, the fix in this patch does not handle function-like macros (namespace MY_LIBRARY_NAMESPACE_FOR_VERSION(42) {).

LGTM if we must... but I don't think we should.

• Added support for inline namespaces and namespace attributes, fixed a typo, added tests.

There's a problem with this patch. Consider the following code:

// In some remote header:
#define SOME_RANDOM_MACRO internal

In D69746#1756448, @gribozavr2 wrote:

The only way I know people apply fixits is with the help of IDEs.

This approach will also introduce false negatives.

While I have no objections against this patch, I wonder whether someone had a chance to ask GCC developers about this? Is it a conscious choice to suggest override when final is present? What's the argument for doing so?

LG.

In D68682#1706581, @poelmanc wrote:

Thanks, from the name that sounds like the perfect place to do it. If cleanupAroundReplacements also is used by clang-format, would we want to make the functionality optional, e.g. via a new bool parameter to cleanupAroundReplacements, a new option in FormatStyle, etc.?

I agree that the test/clang-tidy directory has become hard to navigate. Splitting tests for checks and for infrastructure seems reasonable to me. I personally don't care about specific directory names and file moves/renames are also not a concern, since git is rather good at handling those. I personally strongly prefer long-term improvements over desire to minimize churn.

In D67501#1678962, @gribozavr wrote:

Sorry, I reverted this patch in r372601.

Unfortunately, it makes paths printed in clang-tidy'd diagnostics inconsistent with what -header-filter operates on.

For example, imagine that file-filter.cpp includes header_alias.h, which is a symlink to header.h. The diagnostics printed by clang-tidy refer to header_alias.h, however, after this patch, -header-filter logic calls realpath() and operates on header.h -- making it very difficult for users to understand how exactly to set up filters.

Also, note that paths in diagnostics don't collapse foo/.. or symlinks.

We should only change both sides of path handling simultaneously (paths in diagnostics and paths in filters). However, since getting the user's preferred path is potentially very difficult when symlinks are present, I'm not sure if anything can be done here. Maybe only collapsing foo/.. would be viable?

A couple of drive-by comments.

In D67084#1657534, @xyb wrote:

Thanks. BTW, I can't commit the patch by myself.

In D67056#1657540, @xyb wrote:

Thanks. BTW, I can't commit the patch by myself.

LG if there are no other concerns.

LG with a comment.

LG. Thanks!

Please add full context to the diff. See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

In D67140#1656858, @gribozavr wrote:

Thanks! Yay consistency.

I prefer the term "checker" to refer to individual modules because I feel it is more precise and less ambiguous. In phrases like "malloc check", "make_unique check", it is unclear what does the check -- malloc itself, the caller of malloc, or something else. Therefore, I would be supportive of repainting ClangTidy to also use "checker", but I would want to know what @alexfh thinks about it before we do it in ClangTidy.

Mostly LG, if you've verified that this works. A couple of comments below.

In D66505#1641776, @aaron.ballman wrote:

Given that the alphabetization we want really is based on the string literal, would it make sense to look for that rather than the check name? Adding a few more reviewers for a better mix of opinions.

Ideally, it would be nice to sort the list of registrations by the check name. But given that the call can span two lines, that may be slightly trickier to implement. It's worth giving it a try though ;)

Just FYI, https://bugs.llvm.org/show_bug.cgi?id=43019 is relevant.

LG. Thanks!

In D66042#1627760, @NoQ wrote:

In D66042#1627193, @alexfh wrote:

But without this patch clang seems to have the same two ANALYZE log lines regardless of whether I enable core checkers or not

Yup, it seems as if clang-tidy enables core as long as at least one Static Analyzer checker is enabled (even if it's path-insensitive).

In D66042#1625898, @NoQ wrote:

While we're here: i poked your silencing mechanism a little bit and even though i'm still pretty sure you couldn't have done it perfectly without our help, it sounds as if the only problem you have with it is that the path-sensitive checkers keep running even if only path-insensitive checkers are enabled:

$ clang-tidy test.c -checks=-*,clang-analyzer-unix.cstring.BadSizeArg -- -Xclang -analyzer-display-progress

ANALYZE (Syntax): /Users/adergachev/test/test.c foo                         // <== only this part will actually influence
                                                                            //     the results of analysis in this invocation
ANALYZE (Path,  Inline_Regular): /Users/adergachev/test/test.c foo          // <== however this part is sloooooow

This may be a performance issue for users who want fast analysis but are interested in some path-insensitive Static Analyzer checks (and they don't seem to have a way around that when they limit themselves to clang-tidy's own CLI), but apart from that you indeed seem to be fine.

In D66042#1624081, @NoQ wrote:

+@alexfh because clang-tidy now finally has a way of safely disabling core checkers without causing crashes all over the place! Would you like to take the same approach as we picked in scan-build, i.e. when the user asks to disable a core checker, silence it instead?

One more nit.

In D65065#1617031, @gribozavr wrote:

This suggestion would result another strange behavior: if the user disables cert-err09-cpp because he or she doesn't want to see its reports, the other one (cert-err61-cpp) will still report the issue. So he or she has to disable both (or as many aliases it has).

That seems to be the case regardless of the implementation strategy in this patch.

A general comment: "misc" is a sort of a heap of checks that otherwise don't have a good home. This one would probably better go to bugprone (or maybe there's a relevant CERT or C++ Core Guidelines rule?).

I think it will be a strict improvement to include the check name into the deduplication key (probably after the file and offset and before the message). I don't see any reason to hide this behind a flag or otherwise retain the old behavior. As for expanding the key to include notes and fixes - it's probably good to do this either, and this may help uncover incorrect behavior of some checks. I'd suggest to start with the check name though.

LessClangTidyError only compares location and message, but it could also compare other things like notes, fixes, etc. For the problem outlined in the description of this patch we can probably include the checker name into the key. WDYT?

Seems good now. Haojian, do you have any concerns?

LG. Thanks!

In D33841#1589212, @koldaniel wrote:

Hi, do you have any additional comments?

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.