In D130181#4213779, @LegalizeAdulthood wrote:

I'm not certain that the result of the transformation is more "readable"; is this check intended to aid conformance to a style guide?

This is a great check that I've been meaning to work on for ages but never had time.
It mostly looks good but there are a few issues
I feel there is another issue that this can be very spammy with diagnostics(though clang-tidy is likely suppressing the duplicated ones).
But maybe when we emit a warning for a specified include, we could put it in a set to not warn on that include again

Just a few more points then it should be good to land

LGTM

Also this goes for all your PRs, can you please upload them in future with full context git diff -U999999 should do it. Makes revewing changes easier and removes those Context not available message on phab

Can you just elaborate on what the registry is used for? Is the plan to support potentially dynamically loading HeaderGuardStyle classes.
If thats the case, Then I'd argue we don't need the HeaderGuardBase check class, We can just create a HeaderGuardCheck class that creates an instance of the HeaderGuardStyle based on the configuration options.
Then to maintain backwards compatibility the llvm-header-guard could be an alias with the default style option set to llvm for example.

What's the purpose behind this refactor, If its for 142123, then this should be blocked until a consensus is reached over there?

Given the fact its non-standard, it has caveats and peoples eagerness to blindly enable all checks (or all checks from a module).
I feel this check would likely cause more harm than good.

In D141892#4057722, @ccotter wrote:

2. replace the non _v templates to the _v variants is_same -> is_same_v or the equivalent concept same_as

I'm not convinced this is the correct fix.
The guildline states that tools should flag all variables declared at global or namespace scope.
Therefore we shouldn't be flagging any class static members, no matter if they are public, private or protected.

Extend test cases to check inline namespace, extension namespaces and alias namespaces

Remove unneeded static keyword
Improved fix-it logic if there is a space between the template name and the < token

Assuming the pre-merge is happy, this LGTM

On the topic of Objective-C, they use .m and .M extensions, it may be worth pointing out that this list is case sensitive (I'm assuming that's going to be the case.)

Can you update some of the tests to make use of these new directories as well as update the release notes to mention these changes.

LGTM, just a couple points

Add IgnoreMacros options.
Address comments.

Just 2 more CHECK-FIXES lines need updating.
FWIW the reason its good to be explicit is because FileCheck will start searching from the line where the previous CHECK-FIXES had and stop when it sees line that contains the match.
In this example:

// CHECK-FIXES: [this]() { };

If the there was an error with the fix, but later on in the file, that string was found. FileCheck would assume the CHECK-FIXES directive was found and report success.
That may in turn lead other CHECK-FIXES to fail due to the position of the previous match. But those error message would be slightly more confusing as the expected fix was actually found in the output file (just not in the correct place)
Expanding the check to this:

// CHECK-FIXES: auto explicit_this_capture = [this]() { };

Means we have to match the whole line(which is a lot less likely to appear again later in the file.)

It'd be interesting to see how this handles variadic templates, I have a feeling if it isn't done correctly, there will be a lot of false positives. Can test be added to demonstrate the behaviour.
What happens with code like this

void foo(bar&& B) {
  std::move(B);
}

In D141583#4047975, @carlosgalvezp wrote:

Thanks for reviewing! I can also add that the actual code using the option was removed around 5 years ago, so it's about time :) @njames93 Do you have any concerns with this?

Address comments.

Can you run this through clang format to make sure the pre merge is happy and address those nits

I think this patch needs splitting up with the updates to the tests put in their own NFC patch.

In D135405#4039820, @BigPeet wrote:

Ping.

(Sorry, it's my first time contributing to LLVM and I simply don't know what happens next. Do I need to do anything? Or is it just waiting to get merged at some point?)

You do raise a good point about duplication, Therefore does it make sense to also do the same with the IncludeStyle as lots of checks add new includes.
Though there is a precedent to do away with LLVM/Google style, as clang-format should be responsible for reordering inserted includes.

Can you explain the reasoning of why this approach is better than current approach where checks can use global options(Options.getLocalOrGlobal("HeaderFileExtensions", utils::defaultHeaderFileExtensions())) to access the same information?

I don't see the appear of this check as its a situation that I doubt ever appears in code bases. If there are open source code bases where this is a known problem can you please provide links to them as well as running the run_clang_tidy script over them to verify the changes are good.

In D138655#4012557, @Sockke wrote:

Improved the test file.

In D138655#4007822, @carlosgalvezp wrote:

Now that I think a bit better about this I wonder - does it really make sense that we increase the complexity of the check to cover for cases where code does not compile? If it fails to include a header, there's many other things that can possibly go wrong - should clang-tidy checks in general really be defensive against that? @njames93 WDYT?

LGTM, thank you for the patch

I'm just a little uneasy with the description, this isn't "fixing" the linked issue, its more a workaround. The issue itself is also a bit of a non-issue.

I'm really not sure this is the right approach to solve the problem. If the concern here is leaking on exceptions, then the goalpost should be flagging all calls to global new and recommend a smart pointer factory instead. I think that check could even be made as a cppcoreguideline check

In D90568#3949134, @thakis wrote:

This breaks the build: http://45.33.8.238/linux/92294/step_4.txt

Please take a look and revert for now if it takes a while to fix.

Clang already has a warning -Wunused-variable that is designed for this specific purpose. So unless this is bringing anything enhanced functionality I don't see the need for this check.

Hopefully fixed line endings

In D138505#3944285, @sammccall wrote:

Ideas on testing welcome. Does it make sense to rely on the fact that misc-const-correctness is always slow? :-D

I'd say it doesn't, if the check is ever updated in a way to be more performant it'd be nice if we don't need to change anything hard coded in clangd to enable it to run again.

For what it's worth, the clang tidy check can be configured to disable transformations based off DeMorgan's law.

@sammccall I have a feeling you're gonna want to examine this checks feasibility in clangd.

What is the purpose of this check when we have cppcoreguidelines-special-member-functions.
Granted that check won't emit replacements, however the replacements that this check generates would often not improve performance or potentially introduce a bug.

Lg, but can you address that one nit before landing

Fix comments.

Clean up initializer.

Fix clang-format

Rebased
Changed logic so inner namespaces are marked as nester(which makes much more sense imo)

Fix error.

Fix crash when running over llvm

Fix various crashes on real world code

Address documentation comments

I'm not entirely sure why this belongs in the modernize module given anonymous namespaces have been in c++ forever, maybe its more of a misc check? Also the modernize checks are meant to actually emit fixes(ignore the c arrays check :) ), Right now, this only warn, it doesn't appear to act

The default behaviour of this check should be transform void return types as that's how it has been since the check was first created. Adding an option which defaults to changing this behaviour would be harmful to current users of this check.

I'd also suggest using the IgnoreUnlessSpelledInSource traversal kind here. Given that most of the time these will be instantiated we don't want to warn and emit a fix for each instantiation, Or you could just add unless(isInTemplateInstantiation()) to your matcher.

LGTM, just one small nit.

LGTM, just maybe include a test case where a warning from a clang-tidy check is promoted to an error as well.

Why limit this to just the value type traits, makes sense to also support type type traits.