Passionate test-driven developer
I have commit access to clang-tidy.
Passionate test-driven developer
I have commit access to clang-tidy.
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.
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?
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 from review comments
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.