LG with a couple of comments. Do you need someone to land the patch for you?

Thanks for the patch! Looks generally good. A few comments inline.

In D83223#2147306, @aaron.ballman wrote:

In D83223#2147247, @njames93 wrote:

In D83223#2147072, @aaron.ballman wrote:

 // This is not identified as a license comment as the
// block is followed by code.
void foo();

FWIW: https://github.com/GrammaTech/gtirb-pprinter/blob/master/include/gtirb_pprinter/AttPrettyPrinter.hpp or https://github.com/GrammaTech/gtirb/blob/master/include/gtirb/AuxData.hpp (so there are projects which do not put a newline between the license and code).

Short of creating an AI that understands context it won't be possible to determine the difference between license and general documentation, in any case I feel this heuristic is the safest way to ensure good coverage with minimised risk of inserting the guard in the middle of documentation,

My instinct is that we shouldn't be trying to play those games in the first place and should consider *all* leading comments and empty (whitespace-only) lines as part of the "license" and expect the first significant token to be the header guard. e.g., this isn't about the license at all, it's about whether you can have prose before the header guard or not. It's not uncommon for projects to put prose before header guards, nor is it uncommon for it to go after the header guards. tbh, that feels a bit like an option for the feature rather than an automatic behavior because I could also see a project wanting to enforce a consistent style.

Apologies for the delay! It's sort of a crazy time now =\

In D75184#1914788, @njames93 wrote:

In D75184#1914705, @DmitryPolukhin wrote:

You are absolutely right about current behaviour. Thank you for catching this odd behaviour. I'm not 100% confident what is the right behaviour but my guess is that overriding local option from parent config with global from child folder is better so I implemented it and added corresponding test.

It's a tricky one that, but I thing overriding the local option with the global in the sub directory is the correct way to go about this as well.

Internally we have something similar, but with unconditional inheritance and a way to include other configs. I was thinking about implementing this in the FileOptionsProvider, but decided that this would be an overkill for most real projects. Overall the patch looks good to me, but please update the documentation and add a test to ensure that check options get merged correctly.

In D75441#1901071, @gribozavr2 wrote:

I think my preference is to go with isLanguageVersionSupported() and not use base classes.

+1, I can see this working, but the introduction of new concepts does not seem like it pulls its weight given the tiny amount of boilerplate that they eliminate.

LG

Next step is to plumb this to clang-tidy.

In D69782#1886766, @compositeprimes wrote:

I don't have commit rights (this is my first llvm commit!), so could someone help out? Thanks!

LG

Do you have commit rights or should someone help you committing the patch?

In D72730#1866043, @AlexanderLanin wrote:

On second thought maybe this should be fixed in clang-tidy and not here?
Maybe besides -export-fixes there should be an -export-warnings or just -yaml-export?

A few nits.

Thanks for the update!

LG. Thanks for the fix!

In D72527#1843475, @Eugene.Zelenko wrote:

As I wrote, I don't have GitHub commit access, so I need help with commit.

In D72527#1843427, @Eugene.Zelenko wrote:

Just ping.

A drive-by comment.

How is this different from -Wmissing-prototypes?

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.