LGTM

I will submit it

In D128157#3612741, @myhsu wrote:

You forgot to add "Differential Revison: https://reviews.llvm.org/DXXXXXX" in the commit message, which is how Phabricator identifies the differential to close. I don't think it has anything to do with rebasing.

Thanks for this, like Nathan James this has been pestering me for a while :)

Please provide the name and email address you wish to use on the commit and I will submit.

I can submit after you address additional comments by Nathan James.

Clang-tidy tests and docs have been moved to subdirectories by module, please rebase to main:HEAD

Please add a summary of the fix to the release notes for this check.

This whole check seems weird to me. I mean, almost every use of a standard container could throw std::bad_alloc but we don't insist on a local catch for bad_alloc at every possible throwing call site.

I can submit after you address additional comments by Nathan James.

I pushed this change, but for some reason phabricator doesn't show it and close the review... I wonder if it's because I rebased it?

LGTM

Well, it must have been unrelated to your change or some weird interaction on my machine because I rebased and ran check-clang-extra again and it passed this time.

Rebase against main to get updated docs

I get a test failure when I run with your change:

In D128157#3605829, @jspam wrote:

I see :) Yes, looks like Git's rename detection did its job.

Thanks for the review. Could you or someone else please merge this for me? Author: Joachim Priesner <llvm-project-704996@jspam.de>

OK, I've pushed the link fix and the sorting fix to main so if you rebase again, it should just show your changes to the release notes.

LGTM

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

In D128157#3605504, @jspam wrote:

@LegalizeAdulthood Not sure what exactly you mean by "fold your changes into checkers/cppcoreguidelines/virtual-class-destructor.cpp". The only rebase conflict was in VirtualClassDestructorCheck.cpp. Please let me know if there is still something to do.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Clang-Tidy tests and docs have moved to module subdirectories.

Tests and docs have moved to subdirectories by module name.

Please rebase this and fold your changes into checkers/cppcoreguidelines/virtual-class-destructor.cpp

In D128072#3598634, @aaron.ballman wrote:

This sounds like a sensible direction to me. I spot-checked the changes and they all seem reasonable. Giving my LG, but please wait a bit before landing in case someone else has the chance to spot check as well.

In D126495#3588787, @aaron.ballman wrote:

In D126495#3574115, @LegalizeAdulthood wrote:

In D126495#3571511, @njames93 wrote:

Search engines isn't really an issue, its more people who are using older versions of llvm that try and go to the documentation which turns out is a dead link.
Having said that, I still think the lack of a redirect shouldn't block this change.

Aren't we archiving old versions of the documentation, though?

We do:

https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/
https://releases.llvm.org/11.0.0/tools/clang/tools/extra/docs/
and so on.

Giving my LG more officially now to unblock this, but I'd still appreciate some extra eyes on the Python changes.

Ping. All review comments should be addressed in the latest diff.

In D126495#3571511, @njames93 wrote:

Search engines isn't really an issue, its more people who are using older versions of llvm that try and go to the documentation which turns out is a dead link.
Having said that, I still think the lack of a redirect shouldn't block this change.

In D126495#3570767, @aaron.ballman wrote:

In D126495#3569863, @LegalizeAdulthood wrote:

In D126495#3569029, @njames93 wrote:

It would also be nice if there was a redirect that would dynamically translate the old links to new links.

You can do that with .htaccess, but I don't know if that's considered acceptable in clang documentation.

FWIW, I don't have any idea if this is or isn't acceptable.

• Update add_new_check.py to write out module subdir link in generated header
• Update all links in check headers

I'm discarding this review, as it was just for comment purposes, until I have a proper implementation to discuss.

In D126495#3569029, @njames93 wrote:

In D126495#3568998, @LegalizeAdulthood wrote:

Gentle ping

My previous point about the links in the header files not being updated hasn't been addressed.

Gentle ping

In D126495#3553183, @LegalizeAdulthood wrote:

In D126495#3552887, @njames93 wrote:

Can I ask what the motivation is for this change?

https://discourse.llvm.org/t/clang-tidy-rfc-use-module-name-as-a-directory-for-organizing-files/62499

In D126495#3552887, @njames93 wrote:

Can I ask what the motivation is for this change?

I know this is a rather huge diff, but the part that needs to be reviewed is the python script changes.

• Update add_new_check.py for the case when adding a new check, not just updating the docs
• Rebase ReleaseNotes.rst

Update from review comments

• Add C++ test case for acceptable operator, initializer

• Update from review comments
• Disallow operator, unless inside parentheses

Update from review comments

Another observation:
If some new pattern comes up and fixits aren't recognized for a check, it might be better
to switch to a whitelist for checks with fixits rather than going crazier on the file scraping.

LGTM

Are we waiting on some sort of benchmark before accepting/rejecting this change?

If the only thing holding this up is my comments about following the Law of Demeter, I don't think that should block submitting this change.

LGTM

LGTM
Address one nit and ship.