- User Since
- Aug 23 2021, 6:51 AM (74 w, 6 d)
May 24 2022
Added release note.
May 22 2022
Add another test, to test the specific code sample provided in the GitHub issue (check=cppcoreguidelines-pro-type-member-init).
May 21 2022
The pre-merge checks are failing on the clang-format check. Not sure why it's complaining about the formatting of the lit test files - those files have never complied with the project style and have not been checked for style for as long as I remember. Has this policy changed recently?
Renamed test case to better explain what it is that it's testing.
This screenshot shows the code snippet from the GitHub issue, and Clang-Tidy's behaviour before and after this fix.
Feb 28 2022
Hi @curdeius, have you had the chance to look at this since the last round of comments? The only thing I think this module still needs for now (until a new generic header guard check is developed), is a check that there aren't consecutive underscores in the output.
Feb 21 2022
Feb 13 2022
Just a drive-by comment to say, thank you for taking the time to make this fix. It's a bug I've triggered many times. Great to see it being resolved.
Feb 4 2022
My bad. Test should have called run-clang-tidy with -checks=-some-check in the first test case, and with -config "Checks: -some-check" in the second. In both cases, the disabled check should not appear in the Enabled checks: printout.
Feb 2 2022
Do you reckon it's worth expanding the test: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp to look at this issue?
There isn't much testing of the run-clang-tidy.py at the moment, but it doesn't have to always remain that way.
Jan 29 2022
Jan 26 2022
Jan 25 2022
formNoLintBlocks() - drop the & so the vector must be std::moved in
- Hook up AllowIO and EnableNoLintBlocks to NoLintDirectiveHandler::shouldSuppress()
Jan 23 2022
LGTM besides a couple of nits. I think the diagnostic message should just say what the problem is, not what the fix is - that's what the FixitHint is for.
Every review comment so far should be addressed now, with the exception of the 2 points below.
Pass the NoLintErrors SmallVector all the way through the call stack, instead of storing it in the class
Replace NoLintTokenizer class with a single function, getNoLints()
Instead of defining a new error type for the NoLintDirectiveHandler, use the pre-existing error type clang::tooling::Diagnostic
- remove public getter methods for Type and Pos - just use the fields directly
- consolidate the constructors into one constructor
- add comment about the difference about between Checks = None and Checks = ""
- add back the comment about why negative globs are ignored
- don't lazy compute the glob
Fix errors in comments
Put implementation-related classes in an anonymous namespace
Inline short methods into classes
Store the NoLintDirectiveHandler object directly in ClangTidyContext, instead of doing it through a unique_ptr.
Move AllowIO and EnableNoLintBlocks from ClangTidyContext() back to shouldSuppressDiagnostic().
Jan 21 2022
Thank you very much to both of you for having a look at the patch. Yes, I agree it is a large patch, and I could have done a better job splitting it up into more manageable chunks.
Jan 20 2022
Update according to review (rename NoLintPragmaHandler class to NoLintDirectiveHandler)
Jan 17 2022
Jan 7 2022
Jan 6 2022
Updated according to review comments
- Move NoLintToken and other support classes to the CPP file.
- Create a NoLintPragmaHandler class which owns the cache. Access this class through a PIMPL.
- Use StringSwitch instead of StringMap.
- Store the GlobList object instead of repeatedly constructing temporaries.
- Remove Unknown from the NoLintType enum.
- Add constructors to NoLintToken and NoLintBlockToken that can ensure that the objects are initialized with valid values.
- Move shouldSuppressDiagnostic() into ClangTidyContext. This function took ClangTidyContext as a mutable ref anyway.
This resolves the long-standing gripe I have with clang-tidy raising warnings about GoogleTest macros expanded in my code. Good work.
Do you think we can close issues #44873, #43325, and #31587 now that we have this fix?
Dec 28 2021
Dec 22 2021
Thanks for taking a look. I will update the diff to address your comments. Have a good new years break.
Dec 21 2021
Updated the review's edit permissions. Sorry about that, @kadircet.
Dec 20 2021
Remove unnecessary llvm:: qualification.
Dec 14 2021
Hi @kadircet. Today I fixed the last of the bugs in my caching implementation. I still need to run some tests to quantify the performance improvement before submitting the code for a review.
The problem at the heart of all this is that llvm-header-guard isn't written flexible enough to support non-LLVM project structures.
Dec 8 2021
Hi, just giving a progress update. This is just a early heads-up - no action needed from you at this stage.
Dec 5 2021
The ~6700 instances of // end namespace - do you observe any trend, e.g. are they concentrated in one of the LLVM sub-projects? Is it similar to the situation we have with the readability-identifier-naming check, where a number of sub-projects have disabled it in their sub-directory's .clang-tidy file?
Dec 3 2021
LGTM. The use of mutable with public inheritence is all over the clang-tools-extra code. See class SwapIndex : public SymbolIndex. I don't see why your use would be against the norm.
Dec 2 2021
Update: OK I see you have already changed your patch to make the feature enabled by default.
As far as the performance is concerned, I will have a go at improving it and hope to have something to share next week. How does that sound?
If this is the approach we take, I prefer the logic to be switched around: the NOLINT block feature should be enabled by default. Once (the worst of) this performance issue is resolved, it would be nice from a usability perspective if the user did not have to explicitly opt in to use the feature.
Thanks for the investigation. Just exploring the options...
Nov 30 2021
Will aim to review and come back to you in a couple of days.
Nov 29 2021
Update "iff" comment based on review.
Nothing else that comes to mind that I want to see.
@Quuxplusone are you OK with the latest round of diffs?
I think the primary goal is satisfied - in all cases the cast is identified and a warning is generated.
Nov 27 2021
Nov 26 2021
Thanks for the patch.
Nov 23 2021
What would you say are the key differences between this patch differ and previous attempts, e.g. https://reviews.llvm.org/D72566? How does this patch address the concerns raised in the previous reviews?
Nov 22 2021
Nov 20 2021
I reverted my changes to do with the invalid character substitution. Doing something akin to isAllowedInitiallyIDChar() and isAllowedIDChar() in Lexer.cpp will require converting from char* to UTF32*. Windows complicates things by using its own code page (typically Windows-1252). Will require a lot of careful consideration to implement correctly.
Back out the changes to the "replace invalid characters in an identifier with underscores" feature.
Making this feature work with Unicode characters on different operating systems significantly increases the amount of code change required.
This can always be revisited in another patch.