[PATCH] D115715: [clang-tidy] Fix llvm-header-guard for Windows paths containing drive letter (e.g. C:).

salman-javed-nz added a comment. 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 outp

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:23-40 +namespace { +class VaArgPPCallbacks : public PPCallbacks { +public: + VaArgPPCallbacks(ProTypeVarargCheck *Check) : Check(Check) {} + + void MacroExp

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

salman-javed-nz added a comment. 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

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

salman-javed-nz updated this revision to Diff 398048. salman-javed-nz added a comment. Herald added subscribers: usaxena95, arphaman, mgorny. Updated according to review comments - Move implementation to a CPP file. Use a PIMPL for access. Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

salman-javed-nz added a comment. In D116378#3226780 , @carlosgalvezp wrote: > By the way, the similar problem exists in Clang compiler. I have written in > cfe-dev , > Discourse >

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

salman-javed-nz updated this revision to Diff 398087. salman-javed-nz added a comment. Comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116085/new/ https://reviews.llvm.org/D116085 Files: clang-tools-extra/clang-tidy/CMakeLists.txt clang

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

salman-javed-nz added a comment. Friendly ping. Would be good to get these performance improvements into trunk soon, so that we're not prolonging the time that people are putting up with the current slow implementation. Also, I believe that LLVM 14.0.0 will be up for a release candidate soon,

[PATCH] D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments

salman-javed-nz added a comment. Hi, just giving a progress update. This is just a early heads-up - no action needed from you at this stage. I have a rough prototype that seems promising. The big-O/time complexity is no longer dependent on the number of headers being included and the number of

[PATCH] D115715: [clang-tidy] Fix llvm-header-guard for Windows paths containing drive letter (e.g. C:).

salman-javed-nz added a comment. The problem at the root of all this is that llvm-header-guard isn't written flexible enough to support non-LLVM project structures. See https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp#L44 For a path like `C:

[PATCH] D115715: [clang-tidy] Fix llvm-header-guard for Windows paths containing drive letter (e.g. C:).

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp:57 std::replace(Guard.begin(), Guard.end(), '-', '_'); + std::replace(Guard.begin(), Guard.end(), ':', '_'); Are there other characters we should be sa

[PATCH] D115715: [clang-tidy] Fix llvm-header-guard for Windows paths containing drive letter (e.g. C:).

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp:57 std::replace(Guard.begin(), Guard.end(), '-', '_'); + std::replace(Guard.begin(), Guard.end(), ':', '_'); salman-javed-nz wrote: > Are there other c

[PATCH] D115715: [clang-tidy] Fix llvm-header-guard for Windows paths containing drive letter (e.g. C:).

salman-javed-nz added a comment. In D115715#3192085 , @aaron.ballman wrote: > It could be made to be useful outside of LLVM, but as it stands today, the > check is only intended to be useful for LLVM. If we want to make it useful > outside of LLVM, tha

[PATCH] D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments

salman-javed-nz added a comment. In D108560#3192081 , @kadircet wrote: > Hi @salman-javed-nz ! > Also I am not sure what kind of timelines you have in mind for the > performance improvements, but I think it would be great to have them before > LLVM-14 r

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

salman-javed-nz updated this revision to Diff 395606. salman-javed-nz added a comment. Remove unnecessary `llvm::` qualification. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116085/new/ https://reviews.llvm.org/D116085 Files: clang-tools-extra

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

salman-javed-nz added a comment. Updated the review's edit permissions. Sorry about that, @kadircet. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116085/new/ https://reviews.llvm.org/D116085 ___ cfe-com

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

salman-javed-nz added a comment. Thanks for taking a look. I will update the diff to address your comments. Have a good new years break. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:197 + SourceLocation Loc = FileStartLoc.getLocWithOffset( + s

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/index.rst:347 +// No warnings are suppressed, due to double negation +Foo(bool param); // NOLINT(-google*) }; Would anyone do this on purpose, or is this a user error

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added a comment. What should happen with the following code: // NOLINTBEGIN(google*) // NOLINTEND(google-explicit-constructor) // NOLINTEND(google-runtime-operator) and this code: // NOLINTBEGIN(google-explicit-constructor) // NOLINTBEGIN(google-runtime-operator)

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added a comment. In D111208#3050680 , @carlosgalvezp wrote: >> How should Clang-Tidy behave when mixing check-specific NOLINTBEGIN/ENDs >> with globbed ones? > > I would say keep current behavior - complain that the user wrote something

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353 + if (SuppressionIsSpecific) +*SuppressionIsSpecific = true; } salman-javed-nz wrote: > carlosgalvezp wrote: > > salman-javed-nz

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added a comment. In D111208#3053061 , @carlosgalvezp wrote: > Looking at the code, I see that you use `SuppressionIsSpecific` in order to > separate the errors into 2 error lists: `SpecificNolintBegins` and > `GlobalNolintBegins`. Howev

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added a comment. In D111208#3053370 , @carlosgalvezp wrote: > By the way, is `NOLINTBEGIN/END` expected to work/give errors when the check > name is something else than a real check name? See for example: > > https://godbolt.org/z/b6cbTe

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added a comment. In D111208#3053660 , @carlosgalvezp wrote: > Now, the requirements for a match are stricter (and simpler), making the code > easier to reason about: any NOLINTBEGIN(X) must be matched by an identical > NOLINTEND(X), for

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:446 SuppressionErrors.emplace_back(Error.getValue()); -return false; } carlosgalvezp wrote: > I had to remove these "return false", otherwi

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:407-409 + if (!NolintBegins.empty() && + (NolintBegins.back().second == NolintBracket)) { +NolintBegins.pop_back(); carlosgalvezp

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added a comment. Thanks for all the changes. You've addressed all my questions ad I don't have anything more to add. Let's wait for the reviewers to take a look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/D111208

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:349 + // Allow specifying a few checks with a glob expression. + GlobList Globs(ChecksStr); + if (!Globs.contains(CheckName)) carlosgalv

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/index.rst:314 +are ignored here, as they would effectively re-activate the warning, causing +confusion to the users. There's something about this "causing confusion to the user

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz created this revision. salman-javed-nz added reviewers: aaron.ballman, whisperity, kazu. salman-javed-nz added a project: clang-tools-extra. Herald added subscribers: carlosgalvezp, rnkovacs. salman-javed-nz requested review of this revision. - Fix spelling and grammatical mistakes

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz updated this revision to Diff 381701. salman-javed-nz added a comment. Herald added a subscriber: arphaman. Change double spaces after commas and periods to single space. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112356/new/ htt

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz updated this revision to Diff 381702. salman-javed-nz added a comment. Herald added subscribers: kbarton, nemanjai. Replace curly quote characters (‘ ’ “ ”) with standard straight ones (' "). Replace en-dash (`–`) with standard hyphen (`-`). In both cases, the latter is used more o

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz updated this revision to Diff 381703. salman-javed-nz added a comment. Standardize spelling of "e.g." and "i.e." Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112356/new/ https://reviews.llvm.org/D112356 Files: clang-tools-extra

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz updated this revision to Diff 381704. salman-javed-nz added a comment. Herald added a reviewer: lebedev.ri. Herald added a subscriber: lebedev.ri. Remove repeated words, e.g. "for a a larger user input". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz updated this revision to Diff 381706. salman-javed-nz added a comment. Standardize to US English spelling for words such as "behavior" and "specialization". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112356/new/ https://reviews.

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz updated this revision to Diff 381707. salman-javed-nz added a comment. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. Fix spelling and grammatical mistakes found across the .rst files. // end of patch Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz added a comment. A collection of minor corrections that I don't think deserve separate commits. It shouldn't take much congitive load to remove it all as one patch. But I have used Phabricator's "Update Revision" feature to break out the change into smaller chunks anyway. Repos

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz updated this revision to Diff 381710. salman-javed-nz added a comment. Pre-merge checks failing because patch cannot be applied. Therefore recreating patch to fix this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112356/new/ https

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz added a comment. Thanks for the review. Are you able to commit for me? Salman Javed Thank you very much. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112356/new/ https://reviews.llvm.org/D112356

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz added a comment. In D112356#3082084 , @carlosgalvezp wrote: > Awesome @salman-javed-nz , thanks for fixing the docs! May I ask how did you > create these "smaller commits"? It's very nice to click on each of them and > see only what cha

[PATCH] D112356: [NFC] Tidy up spelling, grammar, and inconsistencies in clang-tidy documentation

salman-javed-nz added a comment. In D112356#3082218 , @Quuxplusone wrote: > Definitely an improvement! I looked at all the changed places and found some > more improvements you can make. I don't need to see this again, though. Thanks for the suggestion

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

salman-javed-nz created this revision. salman-javed-nz added reviewers: alexfh, aaron.ballman, njames93. salman-javed-nz added a project: clang-tools-extra. Herald added subscribers: arphaman, xazax.hun. salman-javed-nz requested review of this revision. Add support for `NOLINTBEGIN` ... `NOLINTEN

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

salman-javed-nz updated this revision to Diff 370293. salman-javed-nz added a comment. Add additional checks in `test/clang-tidy/infrastructure/nolintbeginend.cpp` to check that error suppressions in one file are carried over when the file is `#included` in another file. Repository: rG LLVM

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

salman-javed-nz added subscribers: gonzalobg, toklinke. salman-javed-nz added a comment. @gonzalobg, @toklinke - I think you two might be interested in this because you have made proposals about this feature before. gonzalobg - your Bugzilla ticket: https://bugs.llvm.org/show_bug.cgi?id=3 to

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

salman-javed-nz updated this revision to Diff 371308. salman-javed-nz added a comment. clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp: - Added test to check what happens when NOLINTEND marker is used before NOLINTBEGIN marker (class B1 , line 7).

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

salman-javed-nz added inline comments. Comment at: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:6 + +// NOLINTEND +class B1 { B1(int i); }; aaron.ballman wrote: > Do you think this should be diagnosed as a sign of user confusion with the

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

salman-javed-nz updated this revision to Diff 372150. salman-javed-nz added a comment. Changes in this latest patch: - `LineIsMarkedWithNOLINT()`: Moved `NOLINTBEGIN/END` aspects into a separate function. - `LineIsWithinNOLINTBEGIN()`: A `NOLINTBEGIN/END` region is only considered valid if both

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

salman-javed-nz added a comment. In D108560#2989295 , @aaron.ballman wrote: > Is this syntax used by any other tools? It seems Google have implemented `NOLINTBEGIN` and `NOLINTEND` support in cpplint. I see lines such as `// NOLINTBEGIN(whitespace/line

<    1   2