LG

Thanks for the fix! However, I'm not sure it's possible to correctly rewrite code in all cases where macros are involved. See a couple of motivating examples in the comment.

Sorry for the delay. This patch fell through the cracks. If you're still interested, could you rebase it on top of current HEAD and upload a full diff? Or use the arcanist tool, see https://llvm.org/docs/Phabricator.html.

Looks good!

Feel free to ping patches every week or so. It looks like in this case all the reviewers were swamped with something else at the time.

Looks good! Thanks for the fix! IIUC, this is related to https://bugs.llvm.org/show_bug.cgi?id=34879? Makes sense to specify this in the patch description.

• Updated release notes and default option values in docs

• clang-format
• addressed review comments

Looks good modulo comment.

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.