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

[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

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

[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

[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

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

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

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

[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

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

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

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

[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:407-409 + if (!NolintBegins.empty() && + (NolintBegins.back().second == NolintBracket)) { +NolintBegins.pop_back();

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

[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

[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] 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 : Lines) { +if (isNOLINTFound("NOLINTBEGIN", CheckName, Line, , + )) {

[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

[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

[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

[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

[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

[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

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

[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

[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

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

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

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

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

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/test/clang-tidy/checkers/readability-identifier-naming.cpp:299 // CHECK-FIXES: {{^}} void v_Bad_Base_Method() override {} + void BadBaseMethodNoAttr() {} + // CHECK-FIXES: {{^}} void

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

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

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

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

[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

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

[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

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

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

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

[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

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

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

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

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

[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

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

  1   2   >