[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9ff4f2dfea63: [clang-tidy] Fix #55134 (regression introduced by 5da7c04) (authored by salman-javed-nz). Repository: rG LLVM Github Monorepo

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

salman-javed-nz updated this revision to Diff 431805. salman-javed-nz added a comment. Added release note. (Also taking the opportunity to run the patch through the pre-merge build bot one last time.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

salman-javed-nz added a comment. In D126138#3529820 , @paulaltin wrote: > Thanks for preparing this revision @salman-javed-nz! > > Do you think it could be worth adding a few more test cases to cover this? It > turned out that this issue wasn't actually

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

salman-javed-nz updated this revision to Diff 431223. salman-javed-nz added a comment. Add another test, to test the specific code sample in the GitHub issue (check=cppcoreguidelines-pro-type-member-init). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

salman-javed-nz updated this revision to Diff 431213. salman-javed-nz edited the summary of this revision. salman-javed-nz added a comment. Renamed test case to better explain what it is that it's testing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

salman-javed-nz added a comment. F23151067: Capture.png This screenshot shows the code snippet from the GitHub issue, and Clang-Tidy's behaviour before and after this fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

salman-javed-nz created this revision. salman-javed-nz added reviewers: nridge, paulaltin. salman-javed-nz added a project: clang-tools-extra. Herald added subscribers: carlosgalvezp, xazax.hun. Herald added a project: All. salman-javed-nz requested review of this revision. Herald added a

[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

[PATCH] D120196: [clang-tidy][NFC] Remove Tristate from CachedGlobList

salman-javed-nz accepted this revision. salman-javed-nz added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120196/new/ https://reviews.llvm.org/D120196

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

salman-javed-nz added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119481/new/

[PATCH] D118104: Make run-clang-tidy.py print the configured checks correctly

salman-javed-nz added a comment. My bad. Test should have called clang-tidy with `--checks` in one test case, and with `--config` in the second. In both cases, the disabled check should not appear in the `Enabled checks:` printout. CHANGES SINCE LAST ACTION

[PATCH] D118104: Make run-clang-tidy.py print the configured checks correctly

salman-javed-nz added a comment. In D118104#3292862 , @JesApp wrote: > Well, since this was more of source of confusion than actual incorrect > behaviour, I don't think there should be a test for it. > > In general though, I think the script is complex

[PATCH] D118104: Make run-clang-tidy.py print the configured checks correctly

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

[PATCH] D118519: [clang-tidy] Organize the release notes a little better

salman-javed-nz added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:221-227 Removed checks ^^ Improvements to include-fixer - The improvements are... Small nit, not about this patch, but the

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

salman-javed-nz added a comment. In D116085#3272200 , @nemanjai wrote: > This is causing buildbot failures with `-Werror` (i.e. > https://lab.llvm.org/buildbot/#/builders/57/builds/14322/steps/5/logs/stdio). > Please fix or revert. Thanks for the

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG19eaad94c47f: [clang-tidy] Cache the locations of NOLINTBEGIN/END blocks (authored by salman-javed-nz). Changed prior to commit:

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

salman-javed-nz updated this revision to Diff 402818. salman-javed-nz added a comment. Review comment: `formNoLintBlocks()` - drop the `&` so the vector must be std::moved in Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116085/new/

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

salman-javed-nz updated this revision to Diff 402774. salman-javed-nz added a comment. `ClangTidyContext::shouldSuppressDiagnostic()`: - Hook up `AllowIO` and `EnableNoLintBlocks` to `NoLintDirectiveHandler::shouldSuppress()` `NoLintToken`: -Remove copy ctor and assignment operator. Class is

[PATCH] D117939: [clang-tidy] Add more documentation about check development

salman-javed-nz added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:76 +When you `configure the CMake build `_, +make sure that you enable the ``clang-tools-extra`` project to

[PATCH] D117857: [clang-tidy] Remove gsl::at suggestion from cppcoreguidelines-pro-bounds-constant-array-index

salman-javed-nz accepted this revision. salman-javed-nz added a comment. This revision is now accepted and ready to land. 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.

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

salman-javed-nz added a comment. Every review comment so far should be addressed now, with the exception of the following 2 points. Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.cpp:420 + // file if it is a . + Optional FileName =

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

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:349 + if (Context.shouldSuppressDiagnostic(DiagLevel, Info, SuppressionErrors, + EnableNolintBlocks)) {

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

salman-javed-nz marked an inline comment as done. salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:448 +/// this line. +static std::pair getLineStartAndEnd(StringRef S, size_t From) { + size_t StartPos =

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

salman-javed-nz marked 12 inline comments as done. salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.cpp:63 +// as parsed from the file's character contents. +class NoLintToken { +public: kadircet wrote: >

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

salman-javed-nz marked 4 inline comments as done. salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:84 + bool AllowEnablingAnalyzerAlphaCheckers = false, + bool AllowIO = true, bool

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

salman-javed-nz updated this revision to Diff 402308. salman-javed-nz changed the visibility from "Only User: salman-javed-nz (Salman Javed)" to "Public (No Login Required)". salman-javed-nz added a comment. Pass the `NoLintErrors` SmallVector all the way through the call stack, instead of

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

salman-javed-nz added a comment. 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. One reason this change is so big is because I set myself an ambitious target

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

salman-javed-nz updated this revision to Diff 401576. salman-javed-nz edited the summary of this revision. salman-javed-nz added a comment. Update according to review (rename NoLintPragmaHandler class to NoLintDirectiveHandler) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

salman-javed-nz added a comment. In D116085#3257210 , @carlosgalvezp wrote: > Amazing job @salman-javed-nz ! Here's some initial comments. Reviewing the > `NoLintPragmaHandler.cpp` will take some more time from me. It would have > been easier to see

[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] 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

[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 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

[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] 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

[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( +

[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 ___

[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:

[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

[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,

[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

[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

[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

[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] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

salman-javed-nz accepted this revision. salman-javed-nz added a comment. This revision is now accepted and ready to land. LGTM. The use of `mutable` with public inheritence is all over the clang-tools-extra code. See `class SwapIndex : public SymbolIndex`. CHANGES SINCE LAST ACTION

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

salman-javed-nz added a comment. 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? Repository: rG LLVM

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

salman-javed-nz added a comment. In D108560#3168006 , @ymandel wrote: > In D108560#3167847 , > @salman-javed-nz wrote: > >> With regards to reverting it, is the cat out of the bag? I already see some >> usage

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

salman-javed-nz added a comment. In D108560#3167883 , @aaron.ballman wrote: > In D108560#3167847 , > @salman-javed-nz wrote: > >> If the current code stayed in the main repo, how long could you afford to >>

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

salman-javed-nz added a comment. In D108560#3167847 , @salman-javed-nz wrote: > I can have a go at coming up with a solution where it does caching of the > NOLINT marker positions when a diagnostic is generated in a file. That would > probably help a

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

salman-javed-nz added a comment. Thanks for the investigation. Just exploring the options... With regards to reverting it, is the cat out of the bag? I already see some usage of the NOLINTBEGIN feature in other projects. There's also a NOLINT check globbing feature that builds on top of this,

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

salman-javed-nz added a comment. Will aim to review and come back to you in a couple of days. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113422/new/ https://reviews.llvm.org/D113422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

This revision was automatically updated to reflect the committed changes. Closed by commit rGc7aa358798e6: [clang-tidy] Fix pr48613: llvm-header-guard uses a reserved identifier (authored by salman-javed-nz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

salman-javed-nz updated this revision to Diff 390464. salman-javed-nz added a comment. Update "iff" comment based on review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114149/new/ https://reviews.llvm.org/D114149 Files:

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

salman-javed-nz added a comment. Nothing else that comes to mind that I want to see. @Quuxplusone are you OK with the latest round of diffs? Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:318 // FIXME: This should be a static_cast.

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:63-73 +static clang::CharSourceRange getReplaceRange(const ExplicitCastExpr *Expr) { + if (const auto *CastExpr = dyn_cast(Expr)) { +return

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

salman-javed-nz added a comment. I think the primary goal is satisfied - in all cases the cast is identified and a warning is generated. For the `Widget&` case, a warning is generated but no fixit is offered, but that isn't any worse than the existing C-style cast fixits. It does sound like a

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

salman-javed-nz added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342 + auto w = new int(x); +} carlosgalvezp wrote: > carlosgalvezp wrote: > > Quuxplusone wrote: > > > What about > > > ``` > > >

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

salman-javed-nz accepted this revision. salman-javed-nz added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:335 + const char *str = "foo"; + auto s = S(str); +}

[PATCH] D112881: [clang-tidy] Allow disabling integer to floating-point narrowing conversions for cppcoreguidelines-narrowing-conversions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-narrowingintegertofloatingpoint-option.cpp:12 + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:14: warning: narrowing conversion from 'unsigned long

[PATCH] D112881: [clang-tidy] Allow disabling integer to floating-point narrowing conversions for cppcoreguidelines-narrowing-conversions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-narrowingintegertofloatingpoint-option.cpp:12 + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:14: warning: narrowing conversion from 'unsigned long

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:100-104 + if (isa(CastExpr)) +ReplaceRange = CharSourceRange::getCharRange( +CastExpr->getLParenLoc(), +

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

salman-javed-nz added a comment. Thanks for the patch. Just a little bit of feedback but overall I'm happy with the approach taken. Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:99-106 + CharSourceRange ReplaceRange; + if (isa(CastExpr)) +

[PATCH] D114317: [clang-tidy][WIP] Do not run perfect alias checks

salman-javed-nz added a comment. 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? CHANGES SINCE LAST ACTION

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

salman-javed-nz marked 4 inline comments as done. salman-javed-nz added a comment. 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*`.

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

salman-javed-nz updated this revision to Diff 388699. salman-javed-nz edited the summary of this revision. salman-javed-nz added a comment. Back out the "replace invalid characters in an identifier with underscores" feature. Making this feature work with Unicode characters on different operating

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

This revision was automatically updated to reflect the committed changes. Closed by commit rG625901636134: [clang-tidy] Fix false positive in readability-identifier-naming check… (authored by fwolff, committed by salman-javed-nz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

salman-javed-nz marked an inline comment as not done. salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp:28 +/// Replace invalid characters in the identifier with '_'. +static std::string replaceInvalidChars(StringRef

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

salman-javed-nz marked an inline comment as not done. salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp:28 +/// Replace invalid characters in the identifier with '_'. +static std::string replaceInvalidChars(StringRef

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp:28 +/// Replace invalid characters in the identifier with '_'. +static std::string replaceInvalidChars(StringRef Identifier) { + std::string Fixed{Identifier};

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

salman-javed-nz created this revision. salman-javed-nz added reviewers: whisperity, hokein, aaron.ballman. salman-javed-nz added a project: clang-tools-extra. Herald added subscribers: carlosgalvezp, rnkovacs, xazax.hun. salman-javed-nz requested review of this revision. Fixes

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

salman-javed-nz accepted this revision. salman-javed-nz added a comment. This revision is now accepted and ready to land. LGTM. Nothing more to suggest from my side. Can we allow a few days for the other reviewers to put in their 2c. As for the Bugzilla ticket

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

salman-javed-nz added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:299 // CHECK-FIXES: {{^}} void v_Bad_Base_Method() override {} + void BadBaseMethodNoAttr() {} + // CHECK-FIXES: {{^}} void

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:50 struct ClangTidyStats { - ClangTidyStats() - : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), ErrorsIgnoredNOLINT(0), -ErrorsIgnoredNonUserCode(0),

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

salman-javed-nz added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:329 + + // FIXME: The fixes from ATOverridden should be propagated to the following call + a_vTitem.BadBaseMethod(); The fixes

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:1260 if (Decl->isMain() || !Decl->isUserProvided() || -Decl->size_overridden_methods() > 0) +Decl->size_overridden_methods() > 0 ||

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

salman-javed-nz added a comment. Happy to take a look at this, and do some of the initial review legwork, but let's leave final approval to @aaron.ballman. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113830/new/ https://reviews.llvm.org/D113830

[PATCH] D113450: [clang-tidy] Fix llvm-header-guard so that it works with Windows paths

salman-javed-nz added a comment. @aaron.ballman - I've added the unit test for UNC path as you suggested. Since you've already given the LGTM, I assume you don't need to see the patch again, so I have gone ahead with the commit. Anyway, I'll be around to address any problems if they crop up.

[PATCH] D113450: [clang-tidy] Fix llvm-header-guard so that it works with Windows paths

This revision was automatically updated to reflect the committed changes. Closed by commit rGb4f6f1c9369e: [clang-tidy] Fix llvm-header-guard so that it works with Windows paths (authored by salman-javed-nz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

salman-javed-nz updated this revision to Diff 386047. salman-javed-nz added a comment. Revert to using simple string literals. I was being too clever for my own good with the Twine. I think this should no longer cause ASan issues. WDYT? This was meant to be a simple patch, so sorry for all the

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

salman-javed-nz added a comment. In D113472#3119292 , @jgorbe wrote: > This change is causing asan errors when running the clang-tidy checks under > ASan, most likely because of the reason akuegel pointed out in his comment. > I'm going to revert the

[PATCH] D113450: [clang-tidy] Fix llvm-header-guard so that it works with Windows paths

salman-javed-nz updated this revision to Diff 385780. salman-javed-nz added a comment. Unit tests: - Renamed Samba to SMB - Added test for UNC paths Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113450/new/ https://reviews.llvm.org/D113450

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

salman-javed-nz added a comment. Thanks for the review. I will think some more about your suggestion to look for `//`. Once I have something that works, I will be back for another review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113472/new/

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

This revision was automatically updated to reflect the committed changes. Closed by commit rG00769572025f: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC) (authored by salman-javed-nz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

salman-javed-nz added a comment. I suspect that ensuring that `NOLINT`s are within a comment could be a reasonably large code change. If it is to be robust with `/* multiline comments */` and other shenanigans, then I would probably lean on the compiler to tell us what is and isn't a comment,

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

salman-javed-nz added a comment. clang-tidy looking for `NOLINT` markers but not checking to see that the marker is within a comment, is long-standing behaviour at this point. cpplint has the same behaviour, which explains why clang-tidy's implementation started off this way. That's not to say

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

salman-javed-nz created this revision. salman-javed-nz added a reviewer: carlosgalvezp. salman-javed-nz added a project: clang-tools-extra. Herald added a subscriber: xazax.hun. salman-javed-nz requested review of this revision. Calling clang-tidy on ClangTidyDiagnosticConsumer.cpp gives a

[PATCH] D113450: [clang-tidy] Fix llvm-header-guard so that it works with Windows paths

salman-javed-nz created this revision. salman-javed-nz added reviewers: bkramer, hokein, aaron.ballman. salman-javed-nz added a project: clang-tools-extra. Herald added subscribers: carlosgalvezp, xazax.hun. salman-javed-nz requested review of this revision. Fixes

[PATCH] D112926: run-clang-tidy.py analyze unique files only

This revision was automatically updated to reflect the committed changes. Closed by commit rG0dc856ed20e0: [clang-tidy] run-clang-tidy.py: analyze unique files only (authored by serkazi, committed by salman-javed-nz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D112926: run-clang-tidy.py analyze unique files only

salman-javed-nz added a comment. In D112926#3113206 , @serkazi wrote: > This is now accepted, please feel free to merge on my behalf. Thanks. What's the full name and email you want associated with your commit? CHANGES SINCE LAST ACTION

[PATCH] D112926: run-clang-tidy.py analyze unique files only

salman-javed-nz added a comment. It looks good to me. I don't have any more comments to add - it is a very small code change after all. I have commit access and am happy to commit it on your behalf. However, this would be my first time as a reviewer, and I don't want to be presumptuous and

[PATCH] D112926: run-clang-tidy.py analyze unique files only

salman-javed-nz added a comment. Why does the compile-commands.json have duplicate entries in the first place? Would someone do that on purpose? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112926/new/ https://reviews.llvm.org/D112926 ___

[PATCH] D112864: [clang-tidy] Fix lint warnings in clang-tidy source code (NFC)

This revision was automatically updated to reflect the committed changes. Closed by commit rGade0662c51b5: [clang-tidy] Fix lint warnings in clang-tidy source code (NFC) (authored by salman-javed-nz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D112864: [clang-tidy] Fix lint warnings in clang-tidy source code (NFC)

salman-javed-nz added a comment. In D112864#3098351 , @carlosgalvezp wrote: > Looks great, thanks for fixing! I'm surprised we don't have a CI bot running > these checks post-merge? You would think so, but it looks like automatic checking during CI

[PATCH] D112864: [clang-tidy] Fix lint warnings in clang-tidy source code (NFC)

salman-javed-nz added a comment. No functional change is intended. The majority of clang-tidy check infractions were - readability-identifier-naming - llvm-qualified-auto - llvm-namespace-comment I have also fixed obvious typos wherever I noticed them. Repository: rG LLVM Github Monorepo

[PATCH] D112864: [clang-tidy] Fix lint warnings in clang-tidy source code (NFC)

salman-javed-nz created this revision. salman-javed-nz added reviewers: alexfh, aaron.ballman, carlosgalvezp. salman-javed-nz added a project: clang-tools-extra. Herald added subscribers: abrachet, lebedev.ri, zzheng, kbarton, xazax.hun, nemanjai. Herald added a reviewer: lebedev.ri.

[PATCH] D112596: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters

This revision was automatically updated to reflect the committed changes. Closed by commit rG897402e95988: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters (authored by salman-javed-nz). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D112596: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters

salman-javed-nz added a comment. I have just received commit access, and this will be my first patch that I can commit on my own. I'll be in touch if I run into committing issues. Thanks for the review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D112596: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters

salman-javed-nz added a comment. The documentation looks correct. https://clang.llvm.org/extra/clang-tidy/checks/bugprone-easily-swappable-parameters.html > By default, the following, and their lowercase-initial variants are ignored: > bool, It, Iterator, InputIt, ForwardIt, BidirIt, RandomIt,

[PATCH] D112596: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters

salman-javed-nz added a comment. Pretty sure this is a typo. Curiously the unit tests did not pick it up. I have gone through the liberty of adding a unit test to lock down the correct spellings. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

  1   2   >