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

[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 https://reviews.llvm.org/D118104/n

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

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

[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 https://reviews.llv

[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 https://reviews.llvm.or

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

[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 https://reviews.llvm.o

[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] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

salman-javed-nz marked 5 inline comments as done. 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: > salman-javed-nz wrote: > >

[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 373416. salman-javed-nz marked 2 inline comments as done. salman-javed-nz added a comment. `shouldSuppressDiagnostic()`: - Changed to take a container of diagnostics as an argument. If it finds any stray `NOLINTBEGIN`/`NOLINTEND` markers, it creates

[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 373483. salman-javed-nz added a comment. Minor update to function signatures - Remove arguments that are not absolutely required - Added `const`s This really should have been incorporated in my previous patch, so sorry about the double notification.

[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 373932. salman-javed-nz added a comment. Updated according to review comments. - `test/clang-tidy/infrastructure/nolintbeginend-begin-at-eof.cpp`: new test - `test/clang-tidy/infrastructure/nolintbeginend-end-at-sof.cpp`: new test - Use `llvm::ErrorOr

[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 373944. salman-javed-nz marked 6 inline comments as done. salman-javed-nz added a comment. `lineIsWithinNolintBegin()`: - If the search through the preceding lines returns no active `NOLINTBEGINs`, carry on reading the rest of the file anyway. Repo

[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#3012057 , @aaron.ballman wrote: > Thanks, I think this is getting close! There are two more test cases that I > think are interesting (and should cause any issues, hopefully): > > // NOLINTEND > // CHECK-M

[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#3012755 , @aaron.ballman wrote: > Sorry for not thinking of this sooner, but there is another diagnostic we > might want to consider. > > // NOLINTBEGIN(check) > // NOLINTEND(other-check) > > where the fil

[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 374803. salman-javed-nz added a comment. `lineIsWithinNolintBegin()`: Put `NOLINTBEGIN` comments into 2 buckets: 1. Comments that apply to a specific check - `NOLINTBEGIN(check)` 2. Comments that apply to all checks - `NOLINTBEGIN(*)` and `NOLINTBEGIN

[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 374969. salman-javed-nz added a comment. Pre-merge build error doesn't seem related to my change, but rebasing patch anyway just to be sure. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108560/new/ ht

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

salman-javed-nz updated this revision to Diff 375512. salman-javed-nz marked 4 inline comments as done. salman-javed-nz retitled this revision from "[clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines" to "[clang-tidy] Add support fo

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

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:400-401 + bool SuppressionIsSpecific; + for (const auto &Line : Lines) { +if (isNOLINTFound("NOLINTBEGIN", CheckName, Line, &NolintIndex, + &

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

salman-javed-nz added a comment. Also, the pre-merge build failure is confirmed to be a problem with the build system and not the contents of my patch. https://github.com/google/llvm-premerge-checks/issues/353 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

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

salman-javed-nz added a comment. Yes, I will need your help to commit. Salman Javed m...@salmanjaved.org Thank you very much for the review. Looking back at my initial initial submission a few weeks ago, I can see all the valuable improvements the review process has made. Hopefully people find

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

salman-javed-nz updated this revision to Diff 375685. salman-javed-nz added a comment. Resolving build error due to failed unit test. On some build bots, the clang-tidy diagnostics coming from `error_in_include.inc` get printed BEFORE all other diagnostics, REGARDLESS of the location of the `#i

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

salman-javed-nz added a comment. F19296441: nolintbeginend.cpp.tmp.cpp.msg You can see in the attached file, that when I ran the unit test on a x86_64 Windows 10 PC, the diagnostic from `error_in_include.inc` is printed at the end. However, on this this clang

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

salman-javed-nz created this revision. salman-javed-nz added reviewers: whisperity, 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. The string table `DefaultIgnoredPa

[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 https://reviews.llvm.org/D112596

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

[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 https://reviews.llvm.org

[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 https://reviews.llvm.org/D

[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. salman-javed-

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

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

[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 https://reviews.llvm.org/D112

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

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

[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 https://revie

[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 https://revi

[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 https://bugs.llvm.org/show_bug.cg

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

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

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 https://reviews.llvm.

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

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

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

[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 https://reviews.llvm.

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

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

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

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

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

[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] 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 https://bugs.llvm.or

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

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

[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 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] 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*`. Wind

[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 https://reviews.llvm.org/D1143

[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)) +Repl

[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(), +CastExpr->getSubExprAsWritten()->getBegin

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

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

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

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

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

[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] 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: clang-tools-extra/clan

[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 https://reviews.ll

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

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

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

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

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

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

[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 https://re

[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 https://reviews.llvm.org/D

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

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

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

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

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

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

[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 = S.find_last_

[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)) { ++Context.Stats.Erro

[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 = SrcMgr.getNonBuiltinFilenameF

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

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

[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/ https://review

[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: https://reviews.l

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

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

  1   2   >