[PATCH] D98416: [clang-tidy] Fix cppcoreguidelines-narrowing-conversions false positive

carlosgalvezp added a comment. Took the liberty to land it for you, hope it's ok! :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98416/new/ https://reviews.llvm.org/D98416 ___ cfe-commits mailing list

[PATCH] D144036: [clang-tidy] Add bugprone-non-zero-enum-to-bool-conversion check

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. LGTM, thanks! Would be good to fix the last nit before landing. Comment at: clang-tools-extra/clang-tidy/bugprone/NonZeroEnumToBoolConversionCheck.h:17 + +///

[PATCH] D145865: [clang-tidy] Multiple fixes to bugprone-exception-escape

carlosgalvezp added a comment. There's a lot of changes in this patch (at least 6 as per commit message) - please split into smaller patches fixing 1 problem at a time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145865/new/ https://reviews.llvm

[PATCH] D147929: [clang-tidy] Fix handling of UseAssignment option in cppcoreguidelines-prefer-member-initializer

carlosgalvezp accepted this revision. carlosgalvezp added a subscriber: aaron.ballman. carlosgalvezp added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp:134 +

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

carlosgalvezp created this revision. Herald added subscribers: PiotrZSL, jeroen.dobbelaere, shchenz, kbarton, xazax.hun, nemanjai. Herald added a reviewer: njames93. Herald added a project: All. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald adde

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

carlosgalvezp updated this revision to Diff 513992. carlosgalvezp added a comment. Revert code removal, document deprecation notice instead. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148460/new/ https://reviews.llvm.org/D148460 Files: clang-

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

carlosgalvezp updated this revision to Diff 513994. carlosgalvezp added a comment. Rebase and fix conflicts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148460/new/ https://reviews.llvm.org/D148460 Files: clang-tools-extra/clang-tidy/cppcoregu

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

carlosgalvezp updated this revision to Diff 513995. carlosgalvezp added a comment. Add check to list of aliases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148460/new/ https://reviews.llvm.org/D148460 Files: clang-tools-extra/clang-tidy/cppco

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

carlosgalvezp updated this revision to Diff 513996. carlosgalvezp added a comment. Remove check from list of checks, since we do not add aliases there. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148460/new/ https://reviews.llvm.org/D148460 Fil

[PATCH] D148458: [clang-tidy][NFC] Split bugprone-exception-escape tests

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp:267 const derived *p = &d; -throw p; } catch(base *) { I run into this often as well. If you don't want to get push back during

[PATCH] D148444: [clang-tidy] Prevent `llvmlibc-inline-function-decl` triggering on lambdas

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp:43 + // Consider only functions with an external and visible declaration. + if (const auto *MethodDecl = dyn_cast(FuncDecl)) +if (MethodDecl->getParent()->isLa

[PATCH] D144748: [clang-tidy] Add bugprone-empty-catch check

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp:100 +void EmptyCatchCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedCatchStmt = Result.Nodes.getNodeAs("catch"); + Assert that

[PATCH] D144748: [clang-tidy] Add bugprone-empty-catch check

carlosgalvezp added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst:103 +{ +try +{ Use 2 spaces indentation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

This revision was automatically updated to reflect the committed changes. Closed by commit rGb507bda45523: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148

[PATCH] D142592: [clang-tidy][libc] Add an inline function checker for the libc project.

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. LGTM, thanks for the contribution! Please wait a few days for other reviewers to have a look. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D141144: [clang-tidy][doc] Improve clang-tidy documentation

This revision was automatically updated to reflect the committed changes. Closed by commit rG2706919f9197: [clang-tidy][doc] Improve clang-tidy documentation (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141144/new/ http

[PATCH] D142655: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions options

carlosgalvezp updated this revision to Diff 496346. carlosgalvezp added a comment. Document options now that the documentation improvement patch is merged. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142655/new/ https://reviews.llvm.org/D142655

[PATCH] D142939: Fix handling of -> calls for modernize-use-emplace

This revision was automatically updated to reflect the committed changes. Closed by commit rGa543d840ee0a: Fix handling of -> calls for modernize-use-emplace (authored by BigPeet, committed by carlosgalvezp). Changed prior to commit: https://reviews.llvm.org/D142939?vs=493756&id=496552#toc Rep

[PATCH] D142939: Fix handling of -> calls for modernize-use-emplace

carlosgalvezp added a comment. Pushed, thanks for the contribution! I took the liberty of fixing the `std::vector` thing before pushing :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142939/new/ https://reviews.llvm.org/D142939

[PATCH] D142655: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions options

carlosgalvezp added a comment. Friendly ping @njames93 . Since there are more checks coming up introducing more debt and duplication, I believe we should land this rather soon. I intend to land this by Feb 19th if I don't receive any more feedback. Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D143843: [clang-tidy][NFC] Remove ModernizeTidyModule::getModuleOptions

carlosgalvezp created this revision. Herald added a subscriber: xazax.hun. Herald added a reviewer: njames93. Herald added a project: All. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Most of the options stated

[PATCH] D142655: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions options

carlosgalvezp updated this revision to Diff 496924. carlosgalvezp added a comment. - Rebase. - Fix code directly for the newly introduce check in llvmlibc. We do not need the deprecation process here since the check is brand new. - Rebase the getter functions from ClangTidyCheck.h, they don't see

[PATCH] D143917: [clang-tidy] Clarify bugprone-branch-clone diagnostic message

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. LGTM, thanks for the fix! Personally I don't think such a small change is worth mentioning in the Release Notes, but maybe other reviewers think differently, let's give them a co

[PATCH] D143917: [clang-tidy] Clarify bugprone-branch-clone diagnostic message

carlosgalvezp added a comment. @donat.nagy Are you available to push the commit yourself? Otherwise I can help you with that, please let me know your name and email (must match those on your Github account) for attribution. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D143851: [clang-tidy] Tweak 'rule of 3/5' checks to allow defaulting a destructor outside the class.

carlosgalvezp added a comment. > This is why this option was only available for destructors in the first > place, I imagine. The `cppcoreguidelines` are a bit tricky to work with. Some of the rules are too strict to reasonably apply them in practice. We have brought this up with the authors of

[PATCH] D143996: [clang-tidy][doc] Remove unused variable

carlosgalvezp accepted this revision. carlosgalvezp added a comment. LGTM, thanks for the fix! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143996/new/ https://reviews.llvm.org/D143996 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D143851: [clang-tidy] Tweak 'rule of 3/5' checks to allow defaulting a destructor outside the class.

carlosgalvezp added a comment. > That's not so simple. At work we use clang-tidy for few big projects. I had > to explicitly add to some checks `unless(isExpansionInSystemHeader())` to > gain some performance improvement of clang-tidy. This check is one of them. > Problem is that some matchers o

[PATCH] D143843: [clang-tidy][NFC] Remove ModernizeTidyModule::getModuleOptions

This revision was automatically updated to reflect the committed changes. Closed by commit rG45ddc157ca7c: [clang-tidy][NFC] Remove ModernizeTidyModule::getModuleOptions (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D14384

[PATCH] D144037: [clang-tidy] allow tests to use -config-file instead of -config

carlosgalvezp requested changes to this revision. carlosgalvezp added a comment. This revision now requires changes to proceed. LGTM except the comments left by other reviewers! Marking it as "Request Changes" for visibility. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D144041: [clang-tidy] add primitive types for hungarian identifier-naming (unsigned char and void)

carlosgalvezp accepted this revision. carlosgalvezp 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/D144041/new/ https://reviews.llvm.org/D144041

[PATCH] D144037: [clang-tidy] allow tests to use --config-file instead of --config

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. LGTM, thanks for the fix! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144037/new/ https://reviews.llvm.org/D144037

[PATCH] D144216: [clang-tidy] Extract string header from redundant-string-cstr checker

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp:1 -// RUN: %check_clang_tidy %s readability-redundant-string-cstr %t - -typedef unsigned __INT16_TYPE__ char16; -typedef unsigned __INT32_TYPE__ char

[PATCH] D141569: [clang-tidy] Implement CppCoreGuideline F.18

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. LGTM thanks! Let's give a couple more days for last reviews. It probably needs a rebase since the Release Notes have got a couple more checks. Repository: rG LLVM Github Monor

[PATCH] D141569: [clang-tidy] Implement CppCoreGuideline F.18

carlosgalvezp added a comment. I think we aim at having it strict as default (so someone wanting to comply with the rule "as-is" can just enable the check), and then add options to relax it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141569/new

[PATCH] D144216: [clang-tidy] Extract string header from redundant-string-cstr checker

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. Fair enough! I think we should merge this patch as is first, so that people can benefit from it already, and work through the technical debt afterwards. Repository: rG LLVM Gi

[PATCH] D142655: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions options

This revision was automatically updated to reflect the committed changes. Closed by commit rG5b37cddff8e0: [clang-tidy] Introduce HeaderFileExtensions and ImplementationFileExtensions… (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D144422: [clang-tidy] update docs for new hungarian identifier-naming types (unsigned char and void)

This revision was automatically updated to reflect the committed changes. Closed by commit rGdc43f7107e29: [clang-tidy] update docs for new hungarian identifier-naming types (unsigned… (authored by amurzeau, committed by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[PATCH] D144431: [clang-tidy] Fix readability-identifer-naming Hungarian CString options

carlosgalvezp added a comment. Please document fix in the Release Notes, as people might have adopted to the buggy behavior and should know they need to change to the proper one. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/identifier-naming/hunga

[PATCH] D144431: [clang-tidy] Fix readability-identifer-naming Hungarian CString options

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/identifier-naming/hungarian-notation2/.clang-tidy:115 value: On - - key: readability-identifier-naming.HungarianNotation.Options.TreatStructAsClass -valu

[PATCH] D144510: [clang-tidy] improve readability-identifier-naming hungarian options test

carlosgalvezp accepted this revision. carlosgalvezp 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/D144510/new/ https://reviews.llvm.org/D144510

[PATCH] D144217: [clang-tidy] Fix false-positive in readability-container-size-empty

carlosgalvezp added a comment. Looks good in general, thanks for the fix! I have minor comments. Also, I found the comment on the Github issue interesting: > Perhaps, this can even be generalized to all types whose size() and empty() > are constexpr. Would that be a more scalable approach than

[PATCH] D144217: [clang-tidy] Fix false-positive in readability-container-size-empty

carlosgalvezp added a comment. In D144217#4143554 , @PiotrZSL wrote: > In D144217#4143540 , @carlosgalvezp > wrote: > >>> Perhaps, this can even be generalized to all types whose size() and empty() >>> are const

[PATCH] D144431: [clang-tidy] Fix readability-identifer-naming Hungarian CString options

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. Alright, sounds good, thanks for the clarification! Then I misunderstood the change in the tests and indeed it belonged together in this patch, sorry for the confusion. I have ap

[PATCH] D143996: [clang-tidy][doc] Remove unused variable

carlosgalvezp added a comment. @bjosv Are you able to land the land yourself, or should we do it for you? If so, please provide user and email for Github attribution. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143996/new/ https://reviews.llvm.org/D143996 ___

[PATCH] D144216: [clang-tidy] Extract string header from redundant-string-cstr checker

carlosgalvezp added a comment. In D144216#4135395 , @mikecrowe wrote: >> If you're suggesting that I could use the new header to replace >> declarations of basic_string etc. in other tests then I think that would be >> possible with some careful checki

[PATCH] D143996: [clang-tidy][doc] Remove unused variable

This revision was automatically updated to reflect the committed changes. Closed by commit rG2928746ac3f1: [clang-tidy][doc] Remove unused variable (authored by bjosv, committed by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143996/

[PATCH] D143996: [clang-tidy][doc] Remove unused variable

carlosgalvezp added a comment. In D143996#4147759 , @bjosv wrote: > @carlosgalvezp Sorry, I should have mentioned that I don't have push rights > and that this is my first contribution to llvm. > My GitHub user is: bjosv and email: bjorn.a.svens...@e

[PATCH] D144431: [clang-tidy] Fix readability-identifer-naming Hungarian CString options

This revision was automatically updated to reflect the committed changes. Closed by commit rG87447bedac34: [clang-tidy] Fix readability-identifer-naming Hungarian CString options (authored by amurzeau, committed by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D144510: [clang-tidy] improve readability-identifier-naming hungarian options test

carlosgalvezp added a comment. @amurzeau I'm having some trouble downloading the patch, would you mind trying out a rebase on top of latest master? Otherwise I'll try downloading the raw diff. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144510/n

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

carlosgalvezp created this revision. Herald added subscribers: PiotrZSL, xazax.hun. Herald added a reviewer: njames93. Herald added a project: All. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. A previous patch u

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

carlosgalvezp updated this revision to Diff 519633. carlosgalvezp edited the summary of this revision. carlosgalvezp added a comment. Update commit message with Github issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149899/new/ https://reviews

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp:11 +// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

carlosgalvezp updated this revision to Diff 520151. carlosgalvezp added a comment. Herald added a subscriber: arphaman. - Add test where both command line and config settings are used. - Document that the command-line option overrides the config option, for consistency with other command-line opt

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

carlosgalvezp updated this revision to Diff 520152. carlosgalvezp added a comment. Revert unwanted change to UseColor Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149899/new/ https://reviews.llvm.org/D149899 Files: clang-tools-extra/clang-tidy/

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

This revision was automatically updated to reflect the committed changes. Closed by commit rG26f476286fbc: [clang-tidy] Support SystemHeaders in .clang-tidy (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149899/new/ https

[PATCH] D148458: [clang-tidy][NFC] Split bugprone-exception-escape tests

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. LGTM! > A lot of our test files uses macros to differentiate between specific C++ > standards, why not do that here too? It's not that easy, because the #ifdef lines do not remo

[PATCH] D150071: [clang-tidy] Fix bugprone-assert-side-effect to actually give warnings

carlosgalvezp created this revision. Herald added subscribers: PiotrZSL, xazax.hun. Herald added a reviewer: njames93. Herald added a project: All. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Some time ago a pa

[PATCH] D148995: [clang-tidy] Extract areStatementsIdentical

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. > Separating the changes into two patches would only prolong this process and > potentially delay the completion of this task. I understand the concern, but in practice it's actu

[PATCH] D150071: [clang-tidy] Fix bugprone-assert-side-effect to actually give warnings

This revision was automatically updated to reflect the committed changes. carlosgalvezp marked 2 inline comments as done. Closed by commit rG0d6d8a853a6e: [clang-tidy] Fix bugprone-assert-side-effect to actually give warnings (authored by carlosgalvezp). Changed prior to commit: https://reviews

[PATCH] D150071: [clang-tidy] Fix bugprone-assert-side-effect to actually give warnings

carlosgalvezp added a comment. Thanks for the review! Fixed your comments before landing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150071/new/ https://reviews.llvm.org/D150071 ___ cfe-commits mailin

[PATCH] D148697: [clang-tidy] Handle more cases of functions which should always be noexcept

carlosgalvezp added a comment. First of all thank you for the contribution! I had just a quick look so here's some very preliminary comments, will have more time to deeply review during the weekend: - Unfortunately we cannot simply rename a check like this, since it breaks backwards compatibil

[PATCH] D148697: [clang-tidy] Handle more cases of functions which should always be noexcept

carlosgalvezp added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/noexcept-special-functions.rst:10 +`performance-noexcept-special-functions <../performance/noexcept-special-functions.html>`_ +for more information. Piot

[PATCH] D148793: [WIP][clang-tidy] Implement an include-cleaner check.

carlosgalvezp added a comment. I believe this should be discussed in an RFC. We already have the standalone `include-cleaner` tool, why is that not sufficient? Can it be extended instead? There's also the include-what-you-use tool o

[PATCH] D148458: [clang-tidy][NFC] Split bugprone-exception-escape tests

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp:4 +void throwing_throw_nothing() throw() { +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_throw_nothing

[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape

carlosgalvezp added a comment. I would agree that the fact that a function throws or not can only be found out when analyzing the function definition (it's impossible to know from the declaration). There's also a duplicate diagnostic that I don't see much value on, so I would agree with this pa

[PATCH] D148110: [clang-tidy] Ctor arguments are sequenced if ctor call is written as list-initialization.

carlosgalvezp added a comment. The commit message doesn't really tell me "what" this commit is fixing, it only points to a section of the Standard. It talks about "a false positive" but it doesn't tell what this FP is about. Could you write a little bit more about what the problem is? Preferabl

[PATCH] D148995: [clang-tidy] Extract areStatementsIdentical

carlosgalvezp added a comment. Thanks for the refactoring, great to remove some code duplication! As a reviewer it's not possible for me to confidently review the functional changes to `areStatementsIdentical`, since they don't show side by side as they are in different files. Please perform th

[PATCH] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h:61 +private: + mutable std::optional HasIsPresent; }; njames93 wrote: > PiotrZSL wrote: > > no need for mutable, its used only form Check

[PATCH] D139197: [clang-tidy] Do not warn about redundant static in misc-use-anonymous-namespace

carlosgalvezp added a comment. @lebedev.ri If you are happy with the patch, would you mind approving it? Or do you have additional comments that should be addressed? The patch does not need to be approved by the Code Owner. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D139197: [clang-tidy] Do not warn about redundant static in misc-use-anonymous-namespace

carlosgalvezp added a comment. In D139197#3978174 , @lebedev.ri wrote: > In D139197#3977370 , @carlosgalvezp > wrote: > >> @lebedev.ri If you are happy with the patch, would you mind approving it? Or >> do you

[PATCH] D139197: [clang-tidy] Do not warn about redundant static in misc-use-anonymous-namespace

carlosgalvezp added a comment. @njames93 @JonasToth @LegalizeAdulthood Do you have any comments that I should address? If not, could you approve the patch? To recap: this check is a few days old (I added it myself). I realized there's functionality that shouldn't be here so I'm just removing it

[PATCH] D139197: [clang-tidy] Do not warn about redundant static in misc-use-anonymous-namespace

carlosgalvezp added a comment. In D139197#3978488 , @aaron.ballman wrote: > LGTM, though please add a release note describing the change The original check is just a few days old. It didn't exist in the previous release. Should I still add a comment ab

[PATCH] D139197: [clang-tidy] Do not warn about redundant static in misc-use-anonymous-namespace

carlosgalvezp added a comment. PS thanks a lot for the review! :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139197/new/ https://reviews.llvm.org/D139197 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D139197: [clang-tidy] Do not warn about redundant static in misc-use-anonymous-namespace

This revision was automatically updated to reflect the committed changes. Closed by commit rG7fd838791716: [clang-tidy] Do not warn about redundant static in misc-use-anonymous-namespace (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D139113: [clang-tidy] Fix a couple additional cases in misc-use-anonymous-namespace

carlosgalvezp updated this revision to Diff 481257. carlosgalvezp added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139113/new/ https://reviews.llvm.org/D139113 Files: clang-tools-extra/clang-tidy/misc/UseAnonymousNamespaceCh

[PATCH] D139113: [clang-tidy] Fix a couple additional cases in misc-use-anonymous-namespace

carlosgalvezp added a comment. Friendly ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139113/new/ https://reviews.llvm.org/D139113 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://li

[PATCH] D139113: [clang-tidy] Fix a couple additional cases in misc-use-anonymous-namespace

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/misc/use-anonymous-namespace.cpp:46-48 +// OK +static const int v8{123}; +static constexpr int v9{123}; vingeldal wrote: > Is it really the best behavior to allow these?

[PATCH] D139113: [clang-tidy] Fix a couple additional cases in misc-use-anonymous-namespace

This revision was automatically updated to reflect the committed changes. Closed by commit rG35d9f873e3f2: [clang-tidy] Fix a couple additional cases in misc-use-anonymous-namespace only (authored by carlosgalvezp). Changed prior to commit: https://reviews.llvm.org/D139113?vs=481257&id=482119#t

[PATCH] D139919: use std::optional in ClangTidyCheck

carlosgalvezp requested changes to this revision. carlosgalvezp added a comment. This revision now requires changes to proceed. AFAIK it's preferred to use the LLVM types instead of the Standard types: > When both C++ and the LLVM support libraries provide similar functionality, > and there isn’

[PATCH] D139919: use std::optional in ClangTidyCheck

carlosgalvezp added a comment. In D139919#3991247 , @addr2line wrote: > Sorry, I just saw a lot of changes in the llvm repo that change > llvm::Optional to std::optional, like this one >

[PATCH] D139919: use std::optional in ClangTidyCheck

carlosgalvezp added a comment. In D139919#3991250 , @MaskRay wrote: > In D139919#3991242 , @carlosgalvezp > wrote: > >> AFAIK it's preferred to use the LLVM types instead of the Standard types: >> >>> When both C

[PATCH] D139919: use std::optional in ClangTidyCheck

carlosgalvezp added a comment. Sounds like the change to the YAML parser can potentially affect many other components other than clang-tidy - should that be done in a separate patch? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139919/new/ https://reviews.llvm.org/D139919 ___

[PATCH] D137514: [clang-tidy] add check for capturing lambda coroutines

carlosgalvezp requested changes to this revision. carlosgalvezp added a comment. This revision now requires changes to proceed. Looking good! Some minor comments Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.cpp:40 + + diag(Lam

[PATCH] D137514: [clang-tidy] add check for capturing lambda coroutines

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.cpp:40 + + diag(Lambda->getBeginLoc(), "found capturing coroutine lambda"); +} carlosgalvezp wrote: > Perhaps it would be good wi

[PATCH] D139966: [clang-tidy] Use Python3 for add_new_check.py and rename_check.py

carlosgalvezp created this revision. Herald added a subscriber: xazax.hun. Herald added a reviewer: njames93. Herald added a project: All. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Fixes #58782 Repository:

[PATCH] D139966: [clang-tidy] Use Python3 for add_new_check.py and rename_check.py

carlosgalvezp updated this revision to Diff 482609. carlosgalvezp added a comment. Revert format changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139966/new/ https://reviews.llvm.org/D139966 Files: clang-tools-extra/clang-tidy/add_new_chec

[PATCH] D139966: [clang-tidy] Use Python3 for add_new_check.py and rename_check.py

carlosgalvezp updated this revision to Diff 482611. carlosgalvezp added a comment. Mention change in the Release Notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139966/new/ https://reviews.llvm.org/D139966 Files: clang-tools-extra/clang-tid

[PATCH] D139966: [clang-tidy] Use Python3 for add_new_check.py and rename_check.py

carlosgalvezp updated this revision to Diff 482721. carlosgalvezp added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139966/new/ https://reviews.llvm.org/D139966 Files: clang-tools-extra/clang-tidy/add_new_check.py

[PATCH] D139966: [clang-tidy] Use Python3 for add_new_check.py and rename_check.py

carlosgalvezp updated this revision to Diff 482722. carlosgalvezp added a comment. Wrap to 80 chars. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139966/new/ https://reviews.llvm.org/D139966 Files: clang-tools-extra/clang-tidy/add_new_check.py

[PATCH] D140018: [clang-tidy] Support std::string_view in readability-redundant-string-cstr

carlosgalvezp added a comment. Please document the changes in the Release Notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140018/new/ https://reviews.llvm.org/D140018 ___ cfe-commits mailing list cf

[PATCH] D140018: [clang-tidy] Support std::string_view in readability-redundant-string-cstr

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. LGTM! Perhaps give the other reviewers a couple days to take a look in case I've missed anything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revi

[PATCH] D139966: [clang-tidy] Use Python3 for add_new_check.py and rename_check.py

carlosgalvezp added a comment. In D139966#3999473 , @njames93 wrote: > I'm just a little uneasy with the description, this isn't "fixing" the linked > issue, its more a workaround. The issue itself is also a bit of a non-issue. Thanks for reviewing! Th

[PATCH] D139966: [clang-tidy] Use Python3 for add_new_check.py and rename_check.py

This revision was automatically updated to reflect the committed changes. Closed by commit rG66bf54abb54e: [clang-tidy] Use Python3 for add_new_check.py and rename_check.py (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D13

[PATCH] D117529: [clangd][NFC] Cache ClangTidy check globs to speed up createChecks

carlosgalvezp added a comment. Haven't looked much in detail so apologies if my comment is stupid - can't CachedGlobList be used for this purpose? Should be a one-liner change I think. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117529/new/ http

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

carlosgalvezp added a comment. Code looks great! Would it be worth mentioning the change in the release notes? I also wonder if the check documentation needs some updates. From what i read in the discussion this rule has poor enforcement in the guidelines so perhaps it's good to clarify what ex

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:85 + +bool isLiteralTokenSequence(const MacroInfo *Info) { + return std::all_of(Info->tokens_begin(), Info->tokens_end(), LLVM coding standards s

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:23 -namespace { - -bool isCapsOnly(StringRef Name) { - return std::all_of(Name.begin(), Name.end(), [](const char C) { -if (std::isupper(C) || std::isdigit(

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. Looks reasonable to me! I'm fairly new here so I might not be the most "authoritative reviewer", let me know if you'd like someone else to give the final approval

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

carlosgalvezp added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:11 +`ES.31 `_, and +`ES.32

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

carlosgalvezp added a comment. 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 the diff if the contents had been moved as an NFC patch to a separate file, and then applying th

<    2   3   4   5   6   7   8   9   >