[PATCH] D132461: [clang-tidy] Add cppcoreguidelines-avoid-do-while check

carlosgalvezp added a comment. @njames93 Is there anything else I should address? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132461/new/ https://reviews.llvm.org/D132461 ___ cfe-commits mailing list c

[PATCH] D132461: [clang-tidy] Add cppcoreguidelines-avoid-do-while check

carlosgalvezp added a comment. @njames93 Friendly ping. The patch has addressed all comments and remained unchanged for 2 weeks. I would like to land it latest next week. If you are happy with the patch, please accept the review. Alternatively, please let me know if you intend to continue revie

[PATCH] D132461: [clang-tidy] Add cppcoreguidelines-avoid-do-while check

carlosgalvezp updated this revision to Diff 460419. carlosgalvezp added a comment. Adjust warning message for consistency. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132461/new/ https://reviews.llvm.org/D132461 Files: clang-tools-extra/clang-

[PATCH] D30538: Add documentation for -fno-strict-aliasing

carlosgalvezp added a comment. Herald added a subscriber: jeroen.dobbelaere. Herald added a project: All. Hi, is this commit still valid? Why hasn't it been pushed? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D30538/new/ https://reviews.llvm.org/D30538

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

carlosgalvezp updated this revision to Diff 442015. carlosgalvezp marked 2 inline comments as done. carlosgalvezp added a comment. Address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

carlosgalvezp added a comment. @LegalizeAdulthood Thanks for the review! I've now rebased and addressed your comments. I've also verify the docs with the command you suggested, I was missing `-DLLVM_ENABLE_SPHINX` in the cmake command. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

carlosgalvezp added inline comments. Herald added a reviewer: NoQ. Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-const-correctness.rst:10 +`CppCoreGuidelines ES.25

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

carlosgalvezp updated this revision to Diff 448323. carlosgalvezp added a comment. Rebase onto latest main. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 Files: clang-tools-extra/clang-tidy/cppcoregu

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

carlosgalvezp added a comment. @njames93 Would you mind reviewing? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 ___ cfe-commits mailing list cfe-comm

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

carlosgalvezp added a comment. Great feedback, thanks! I had some ideas on how to go around the issues, looking forward to your thoughts. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp:103-104 +struct WithAlias { +

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

carlosgalvezp updated this revision to Diff 448965. carlosgalvezp added a comment. - Display type information in the diagnostic. - Rebase onto latest main. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

carlosgalvezp added a comment. @njames93 I've updated the patch with extra type information, let me know if you think it's good enough! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 __

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

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

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

carlosgalvezp added a comment. Ok, thanks for the explanation! I'm mostly interested on the warning message, we've had situations before where the warning describes the problem **and** the solution, which can easily lead to confusion. From the tests I can see the message is quite generic "use a

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp added a comment. Thanks a lot @tonic for your update and for the work done in the last months ! It's indeed very sad to hear the news. It's a pity that the work will probably be duplicated in many local forks with sub-optimal solutions instead of a centralized, high-quality, peer-

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp added a comment. In D112730#3316646 , @carlosgalvezp wrote: > Thanks a lot @tonic for your update and for the work done in the last months > ! It's indeed very sad to hear the news. It's a pity that the work will > probably be duplicated

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

carlosgalvezp added a comment. @LegalizeAdulthood I've addressed your comments, is there anything that should be fixed before landing the patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 __

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

carlosgalvezp updated this revision to Diff 444633. carlosgalvezp added a comment. - Rebase onto latest main. - Remove references to std::reference_wrapper as alternative suggestions, as per: https://github.com/isocpp/CppCoreGuidelines/issues/1919 Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

carlosgalvezp added a comment. Ping to reviewers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:/

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

carlosgalvezp added a comment. Herald added a subscriber: jeroen.dobbelaere. Hi! I've been following this issue about aliases in clang-tidy through different bug reports and commit attempts. I see that the author abandoned this commit without an explanation. What's the reason? Where can I follo

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

carlosgalvezp added a comment. Fully agree. Such an opt-in feature shouldn't break things. I'd be happy to implement it if people think it's a good idea! Would need some hand-holding though, never touched the LLVM codebase before :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D110493: [clang-tidy] Fix bug 51790 in readability-uppercase-literal-suffix

carlosgalvezp created this revision. carlosgalvezp added reviewers: llvm-commits, cfe-commits, steveire, lebedev.ri, alexfh, alexfh_. carlosgalvezp added a project: clang-tools-extra. Herald added a subscriber: xazax.hun. carlosgalvezp requested review of this revision. A bisect determined that t

[PATCH] D110493: [clang-tidy] Fix bug 51790 in readability-uppercase-literal-suffix

carlosgalvezp added a comment. I personally think this fix should happen when doing the match, not when analyzing the matches. This is my first patch to LLVM and I'm not knowledgeable in AST so I don't really know how to go about that :) Please come with suggestions if there's a better way to d

[PATCH] D110493: [clang-tidy] Fix bug 51790 in readability-uppercase-literal-suffix

carlosgalvezp updated this revision to Diff 375242. carlosgalvezp added a comment. Fixed review comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110493/new/ https://reviews.llvm.org/D110493 Files: clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp clang

[PATCH] D110493: [clang-tidy] Fix bug in readability-uppercase-literal-suffix

carlosgalvezp added a comment. Awesome, thanks a lot for the review! :) Sure, you can use the following to commit on my behalf, since I don't think I have such permissions: Carlos Galvez carlosgalv...@gmail.com CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110493/new/ https://reviews.

[PATCH] D110493: [clang-tidy] Fix bug in readability-uppercase-literal-suffix

carlosgalvezp added a comment. Super, thanks! It went really smooth :) Newbye question: I noticed that `[clang-tidy]` is no longer part of the commit message, I thought the intention was to keep it to more easily glance through the commit history? CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D110493: [clang-tidy] Fix bug in readability-uppercase-literal-suffix

carlosgalvezp added a comment. Thanks for the clarification, makes sense! I got the notification about the build failure, thanks for fixing. I actually added it in the first place but then noticed that checkers typically didn't include any STL header so I thought maybe there was a restriction o

[PATCH] D110614: [clang-tidy] Fix false positive in cppcoreguidelines-virtual-class-destructor

carlosgalvezp created this revision. carlosgalvezp added reviewers: mgartmann, aaron.ballman, alexfh, alexfh_. carlosgalvezp added a project: clang-tools-extra. Herald added subscribers: shchenz, kbarton, xazax.hun, nemanjai. carlosgalvezp requested review of this revision. Incorrectly triggers fo

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp updated this revision to Diff 375545. carlosgalvezp retitled this revision from "[clang-tidy] Fix false positive in cppcoreguidelines-virtual-class-destructor " to "[clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor ". carlosgalvezp edited the sum

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp updated this revision to Diff 375546. carlosgalvezp added a comment. Rename Class to Struct CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110614/new/ https://reviews.llvm.org/D110614 Files: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp updated this revision to Diff 375549. carlosgalvezp added a comment. -Remove "public" when checking for base virtual destructor, it's not relevant. -More consistent naming. -Fix formatting. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110614/new/ https://reviews.llvm.or

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp added a subscriber: whisperity. carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp:26 + ast_matchers::internal::Matcher InheritsVirtualDestructor = + hasAnyBase(hasType(cxxRecordDe

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:216-218 +template +struct TemplatedDerived : PublicVirtualBaseStruct { +}; aaron.ballman wrote: > I think there are more i

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:216-218 +template +struct TemplatedDerived : PublicVirtualBaseStruct { +}; aaron.ballman wrote: > whisperity wrote: > > ca

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp marked 5 inline comments as done. carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:208 +// Forward declarations +// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp updated this revision to Diff 376116. carlosgalvezp added a comment. Added additional required tests. I might take some more time to fix the matcher since I need to get acquainted with AST, clang-query, etc, etc CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110614/new/ h

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp added a comment. By the way, there is a compiler warning that is identical to this check. Wouldn't it make sense to just make both checks identical? Otherwise there's risk that they conflict one another: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclCXX.cpp

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp added a comment. So for some reason, in Sema, the following returns True: `dtor->isVirtual() ` However the clang-tidy matcher : `has(cxxDestructorDecl(isPublic(), isVirtual())),` Doesn't match this, and that's why we get the FPs. Why would that be, isn't `isVirtual` doing the sa

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp added a comment. Ahh, it seems that for templates, the class has this AST: |-ClassTemplateDecl line:7:8 Derived | |-TemplateTypeParmDecl col:20 typename depth 0 index 0 T | |-CXXRecordDecl line:7:8 struct Derived definition | `-ClassTemplateSpecializationDecl line:7:8 st

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp updated this revision to Diff 376139. carlosgalvezp added a comment. Moved the condition of public-virtual / protected-non-virtual to the check stage instead of the match stage. This way is more similar to the equivalent Sema check and it passes all the tests. Let me know what you

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp updated this revision to Diff 376168. carlosgalvezp added a comment. Created custom matcher and moved the logic from check to match stage. Documented why we can't simply match a CXXDestructorDecl. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110614/new/ https://reviews.l

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp added a comment. Thanks for the feedback @whisperity , I'm learning so many new things :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110614/new/ https://reviews.llvm.org/D110614 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp marked 3 inline comments as done. carlosgalvezp added a comment. Hi! Are there any more issues I should address? It would be nice to get it merged since it fixes quite a few FPs. Tagging also @mgartmann as original author of this check in case he wants to comment. Anyway this che

[PATCH] D111041: Remove 'IgnoreDestructors = true' from cppcoreguidelines-explicit-virtual-functions

carlosgalvezp created this revision. carlosgalvezp added reviewers: aaron.ballman, alexfh, lewmpk. Herald added subscribers: shchenz, kbarton, nemanjai. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. This requirement was introduced in the C++ Core guide

[PATCH] D111041: [clang-tidy] Remove 'IgnoreDestructors = true' from cppcoreguidelines-explicit-virtual-functions

carlosgalvezp added a comment. I wonder why I'm not getting automated pre-merge builds? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111041/new/ https://reviews.llvm.org/D111041 ___ cfe-commits mailing

[PATCH] D111041: [clang-tidy] Remove 'IgnoreDestructors = true' from cppcoreguidelines-explicit-virtual-functions

carlosgalvezp added a comment. Thanks for the input! > release note mentions Sure thing! > documentation changes As we discussed in the separate mail thread, there is no documentation at all as to what default values an alias/main check has. Where should I put that? Or do you mean that now t

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp added a comment. Thanks for your input @mgartmann ! No apologies needed, nothing is ever perfect on the first release :) Just wanted to check if you would agree with the fix or if there's some detail I've missed. I'll add the tests asked by @whisperity CHANGES SINCE LAST ACTION

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

carlosgalvezp updated this revision to Diff 377100. carlosgalvezp added a comment. Added test where an alias to the class template is created. @whisperity let me know if that's what you meant! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110614/new/ https://reviews.llvm.org/D110614 F

[PATCH] D112334: [clang-tidy] Suppress readability-static-access-through-instance for CUDA built-in variables

carlosgalvezp added a comment. Thanks for the review! I forgot to mention the Differential Revision in the commit message after pushing so this review stays open, is there any way I can add it now in some other way? I suppose we don't want force-push? CHANGES SINCE LAST ACTION https://review

[PATCH] D112334: [clang-tidy] Suppress readability-static-access-through-instance for CUDA built-in variables

carlosgalvezp added a comment. @whisperity Understood, thanks for the help! I'll take more care to remember the reference next time :) I added it as a comment in Github for reference so there's at least some traceability. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112334/new/ https

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:328 Opts["cert-str34-c.DiagnoseSignedUnsignedCharComparisons"] = "false"; +Opts["cert-err33-c.CheckedFunctions"] = CertErr33CCheckedFunctions; return Options; -

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

carlosgalvezp added a comment. > I was not sure how the aliasing is to be handled if the check is not exactly > the same as the original. I agree that the alias situation is a bit of a mess. I'll leave it to people with stronger opinion/experience. Repository: rG LLVM Github Monorepo CHANG

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

carlosgalvezp added a comment. I've recently fiddled with the `GlobList` to enable globbing in `NOLINT` expressions, so I have it quite fresh and I think it could easily be re-used here. I'd be happy to implement this feature, should I (can I?) continue on this patch or create a brand new one?

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

carlosgalvezp added a comment. On the other hand, switching from Regex to Glob will be quite a breaking change - everyone has `.*` in their config file, which will silently stop to work when switching to Glob and suddenly people won't get their headers linted. What do you think? Repository:

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

carlosgalvezp added a comment. Also, the new header filter expresion would need to be: `HeaderFilter: '*/mydir/*' instead of `HeaderFilter: 'mydir'` Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D34654/new/ https://reviews.llvm.org/D34654

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

carlosgalvezp added a comment. > everyone has .* in their config file, which will silently stop to work Nevermind this. `.*` treated as a glob will turn into the regex `..*` which is equivalent. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D34654/new/ https://re

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

carlosgalvezp created this revision. carlosgalvezp added a project: clang-tools-extra. Herald added subscribers: libc-commits, arphaman, zzheng, kbarton, xazax.hun, mgorny, nemanjai. Herald added a project: libc-project. carlosgalvezp requested review of this revision. Herald added subscribers: cf

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

carlosgalvezp updated this revision to Diff 383034. carlosgalvezp added a comment. Fixed typo. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112720/new/ https://reviews.llvm.org/D112720 Files: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tools-extra/clang-tidy

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

carlosgalvezp added a comment. Here's a patch that uses Globs, let me know how you like it! https://reviews.llvm.org/D112720 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D34654/new/ https://reviews.llvm.org/D34654 ___ c

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

carlosgalvezp updated this revision to Diff 383036. carlosgalvezp added a comment. Fixed formatting of release notes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112720/new/ https://reviews.llvm.org/D112720 Files: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

carlosgalvezp added a comment. In D112720#3093636 , @lebedev.ri wrote: > Not in favor of general directions like this. > Does it not break the existing uses (existing `.clang-tidy`'s e.g.) Yep, it's a breaking change. The purpose of this patch is mostly

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

carlosgalvezp added a comment. Just to clarify, this patch is the solution proposed by @alexfh here: https://reviews.llvm.org/D34654#3022728 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112720/new/ https://reviews.llvm.org/D112720 ___ cfe-c

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp created this revision. carlosgalvezp added reviewers: aaron.ballman, whisperity. carlosgalvezp added a project: clang-tools-extra. Herald added subscribers: armkevincheng, jsmolens, eric-k256, arphaman, rnkovacs, kbarton, xazax.hun, mgorny, nemanjai. Herald added a reviewer: sjarus.

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp updated this revision to Diff 383058. carlosgalvezp added a comment. Specify C++14 in the module description. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112730/new/ https://reviews.llvm.org/D112730 Files: clang-tools-extra/clang-tidy/CMakeLists.txt clang-tools-extr

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp updated this revision to Diff 383059. carlosgalvezp added a comment. Fix ordering in check list. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112730/new/ https://reviews.llvm.org/D112730 Files: clang-tools-extra/clang-tidy/CMakeLists.txt clang-tools-extra/clang-tidy/

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp updated this revision to Diff 383087. carlosgalvezp added a comment. Update release notes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112730/new/ https://reviews.llvm.org/D112730 Files: clang-tools-extra/clang-tidy/CMakeLists.txt clang-tools-extra/clang-tidy/ClangT

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

carlosgalvezp added a comment. In D112720#3094163 , @lebedev.ri wrote: > As long as this blankedly breaks/regresses existing configs it's a > non-starter i think. I see, thanks for the input! I'm curious if there are any deprecation mechanisms for cla

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

carlosgalvezp added a comment. Makes total sense, thanks for the suggestion! I'll wait for more reviewers to see if this behavior is what we want at all. If so I can revert the breaking changes and introduce that option. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112720/new/ https:

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp added a comment. > "C++14 Guidelines"? So it's always and definitely C++14 specific? To my knowledge, AUTOSAR handed over the work to MISRA and the current direction is that MISRA will merge AUTOSAR C++14 and older MISRA revisions into a brand-new MISRA release, coming up 202x (Th

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp updated this revision to Diff 383259. carlosgalvezp added a comment. Removed unnecessary dependency in CMakeLists.txt CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112730/new/ https://reviews.llvm.org/D112730 Files: clang-tools-extra/clang-tidy/CMakeLists.txt clang-to

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp added a comment. Also, I've sent a mail to AUTOSAR directly asking for consent, so there's no doubt. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112730/new/ https://reviews.llvm.org/D112730 ___ cfe-commits mailing list cfe-co

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp added a comment. > people DO incorporate clang-tidy into their own commercial offerings Gna, that //can// be a problem. I wonder if in that case it would be possible to add a few lines into the `LLVM Exceptions` part of the license. If it's too much of a hassle I guess I'll need t

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp added a comment. In D112730#3097091 , @tstellar wrote: > @carlosgalvezp Did you write this patch or did you get it from someone else? @tstellar I wrote it myself (following the existing pattern for the other modules in the llvm-project rep

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

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. Looks great, thanks for fixing! I'm surprised we don't have a CI bot running these checks post-merge? Since this is my first review it's probably good to wait for other reviewers

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

carlosgalvezp added a comment. So after some thoughts, I came up with the following plan: - This patch adds: `HeaderFilter`, `HeaderFilterMode`. `HeaderFilter` is intended to replace `HeaderFilterRegex` and we inform that `HeaderFilterRegex` is deprecated and will be removed in 2 releases. Howe

[PATCH] D112913: Misleading bidirectional detection

carlosgalvezp requested changes to this revision. carlosgalvezp added a comment. This revision now requires changes to proceed. Nice addition! Please add this check to the documentation (list of available checks + individual page with the documentation for this check), plus mention in the clang-

[PATCH] D112913: Misleading bidirectional detection

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp:61 +CheckFactories.registerCheck( +"misc-misleading-bidirectional"); } Nit: please keep alphabetical order in the list of jobs. Repository: rG

[PATCH] D112914: Misleading identifier detection

carlosgalvezp requested changes to this revision. carlosgalvezp added a comment. This revision now requires changes to proceed. - Add check to list of checks: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/docs/clang-tidy/checks/list.rst - Mention check in the Release Notes: ht

[PATCH] D112914: Misleading identifier detection

carlosgalvezp added a comment. Looks good, I guess the license issue still needs to be sorted out? Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-misleading-identifier.rst:6 + +Finds identifiers that contain Unicode characetrs with right-to-left direction, +which ca

[PATCH] D112914: Misleading identifier detection

carlosgalvezp resigned from this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. Ok! I don't really know what applies when you take //part// of a file, so I'll leave that up to people who know. I don't know how to remove the "Requested changes" from here

[PATCH] D113249: Bump CUDA version to 11.5

carlosgalvezp created this revision. Herald added subscribers: asavonic, dexonsmith, hiraditya, yaxunl, jholewinski. carlosgalvezp requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Following the pattern used for 11.4: https:

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

carlosgalvezp added a comment. In D113249#3110954 , @Hahnfeld wrote: > I'm not sure if it's actually correct to advertise full support for CUDA > 11.5, but I didn't look into exact changes since 11.4 Good point. I was confused about the fact that 11.4 i

[PATCH] D112913: Misleading bidirectional detection

carlosgalvezp added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:105 + + Inspect string literal and comments for unterminated bidirectional Unicode + characters. Nit: Inspects CHANGES SINCE LAST ACTION https://reviews.llvm.org/D11291

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

carlosgalvezp updated this revision to Diff 385284. carlosgalvezp added a comment. Updated BuilintsNVPTX.def CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113249/new/ https://reviews.llvm.org/D113249 Files: clang/include/clang/Basic/BuiltinsNVPTX.def clang/include/clang/Basic/Cuda.

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

carlosgalvezp added a comment. > - The driver needs to enable ptx75 when it constructs cc1 command line in > clang/lib/Driver/ToolChains/Cuda.cpp @tra Haven't I already done it in line 712? Or where should I enable it? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113249/new/ https://

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

carlosgalvezp updated this revision to Diff 385286. carlosgalvezp added a comment. Update release notes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113249/new/ https://reviews.llvm.org/D113249 Files: clang/include/clang/Basic/BuiltinsNVPTX.def clang/include/clang/Basic/Cuda.h c

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

carlosgalvezp created this revision. carlosgalvezp added reviewers: aaron.ballman, whisperity. Herald added subscribers: rnkovacs, xazax.hun. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Currently it's hidden in

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

carlosgalvezp added a comment. @Hahnfeld Are you satisfied with the replies to your questions? If so I can go ahead and merge. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113249/new/ https://reviews.llvm.org/D113249 ___ cfe-commits mailing

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

carlosgalvezp added a comment. Awesome, thanks a lot for the reviews! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113249/new/ https://reviews.llvm.org/D113249 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cg

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

This revision was automatically updated to reflect the committed changes. Closed by commit rG7ecec3f0f521: [CUDA] Bump supported CUDA version to 11.5 (authored by carlosgalvezp). Changed prior to commit: https://reviews.llvm.org/D113249?vs=385286&id=385717#toc Repository: rG LLVM Github Mono

[PATCH] D110155: [OpenCL] Allow optional __generic in __remove_address_space utility

carlosgalvezp added a comment. Hi! I believe this commit broke a number of CI jobs, would you be able to look into it? https://lab.llvm.org/buildbot/#/builders/52/builds/12263 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110155/new/ https://revi

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

carlosgalvezp added a comment. Hmm, this sounds a bit hacky. I noticed a similar pattern in tests. I think it deteriorates readability a bit. Can we make the detection of the tag more robust? Right now we check if a line contains NOLINTBEGIN/END. Why not check if it contains "// NOLINTBEGIN"? (

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

carlosgalvezp added a comment. > If it is to be robust with /* multiline comments */ Does it have to be? I don't recall that we have unit tests for that. I would personally restrict the usage to only one way to write the line. I think it's not hard for users to adapt to that, plus it encourage

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

carlosgalvezp added a comment. In D112730#3116281 , @tstellar wrote: > @carlosgalvezp The LLVM Foundation Board will conduct a legal reivew of this > patch. Would you be able to share any information you have about the license > or usage restrictions f

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

carlosgalvezp added a comment. Great! I'd be open to supporting `/*` as well if people really need it. But so far that use case is not documented nor tested, so I think we shouldn't add new functionality if it's not needed. It can be added in the future if someone has a compelling case. Repos

[PATCH] D113518: [clang] Create delegating constructors even in templates

carlosgalvezp requested changes to this revision. carlosgalvezp added a comment. This revision now requires changes to proceed. Amazing, thanks a lot for fixing this long-standing issue! I have a couple comments. I'm not familiar at all with the Sema part so someone that knows should look at it

[PATCH] D113518: [clang] Create delegating constructors even in templates

carlosgalvezp added a comment. Also, what's with the pre-merge test, is it related? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113518/new/ https://reviews.llvm.org/D113518 ___ cfe-commits mailing list

[PATCH] D113518: [clang] Create delegating constructors even in templates

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:374 { PositiveSelfInitialization() : PositiveSelfInitialization() {} }; fwolff wrote: > carlosgalvezp wrote: > > Not reall

[PATCH] D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks

carlosgalvezp added a comment. I was a bit confused as well about the ClangSA support in clang-tidy. What's the current status? Is clang-tidy running *all* checks from StaticAnalyzer? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64454/new/ https://reviews.llvm.o

[PATCH] D113518: [clang] Create delegating constructors even in templates

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. Looks great, thanks! I'm not comfortable reviewing the Sema part, maybe someone else can have a look? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113518/new/ https://

  1   2   3   4   5   6   7   8   9   >