[PATCH] D131780: [clang-tidy] Do not trigger cppcoreguidelines-avoid-const-or-ref-data-members on lambda captures

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

[PATCH] D131780: [clang-tidy] Do not trigger cppcoreguidelines-avoid-const-or-ref-data-members on lambda captures

This revision was automatically updated to reflect the committed changes. Closed by commit rG3fd4213059a4: [clang-tidy] Do not trigger cppcoreguidelines-avoid-const-or-ref-data-members… (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D131780: [clang-tidy] Do not trigger cppcoreguidelines-avoid-const-or-ref-data-members on lambda captures

carlosgalvezp updated this revision to Diff 453536. carlosgalvezp added a comment. Fix typo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131780/new/ https://reviews.llvm.org/D131780 Files:

[PATCH] D131780: [clang-tidy] Do not trigger cppcoreguidelines-avoid-const-or-ref-data-members on lambda captures

carlosgalvezp added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp:191 + auto r5 = []{}; +} njames93 wrote: > njames93 wrote: > > Can you add some cases with implicit capture (using

[PATCH] D131780: [clang-tidy] Do not trigger cppcoreguidelines-avoid-const-or-ref-data-members on lambda captures

carlosgalvezp updated this revision to Diff 453535. carlosgalvezp added a comment. Update implicit lambda captures to use the captured variables inside the lambdas. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131780/new/

[PATCH] D131780: [clang-tidy] Do not trigger cppcoreguidelines-avoid-const-or-ref-data-members on lambda captures

carlosgalvezp updated this revision to Diff 453277. carlosgalvezp added a comment. Added unit test for implicit capture (ref and value) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131780/new/ https://reviews.llvm.org/D131780 Files:

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

carlosgalvezp added a comment. In D131386#3723164 , @aaron.ballman wrote: > In D131386#3723144 , @carlosgalvezp > wrote: > >>> but removing those options can break people's .clang-tidy config files >> >> Aren't

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

carlosgalvezp added a comment. > but removing those options can break people's .clang-tidy config files Aren't there deprecation mechanisms? I think trying to be backwards compatible across all possible versions can lead to suboptimal solutions and make the tool harder to use and hard/slow to

[PATCH] D131780: [clang-tidy] Do not trigger cppcoreguidelines-avoid-const-or-ref-data-members on lambda captures

carlosgalvezp updated this revision to Diff 452391. carlosgalvezp added a comment. Test also capture by reference. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131780/new/ https://reviews.llvm.org/D131780 Files:

[PATCH] D131780: [clang-tidy] Do not trigger cppcoreguidelines-avoid-const-or-ref-data-members on lambda captures

carlosgalvezp added a comment. In D131780#3719881 , @Eugene.Zelenko wrote: > Please mention changes in Release Notes. Thanks for the quick review, @Eugene.Zelenko ! This check was newly added a couple of days ago (as per the "New checks" section in

[PATCH] D131780: [clang-tidy] Do not trigger cppcoreguidelines-avoid-const-or-ref-data-members on lambda captures

carlosgalvezp created this revision. Herald added subscribers: shchenz, kbarton, xazax.hun, nemanjai. Herald added a project: All. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Lambdas are implemented as regular

[PATCH] D131678: hicpp-signed-bitwise - Return location of the operand (and not of the operator beginning)

carlosgalvezp added a comment. In D131678#3716213 , @vladimir.plyashkun wrote: > Hi @carlosgalvezp > Yes, sorry, maybe screenshot and example from issue in our bugtracker is not > the best one. > You can check it on this sample

[PATCH] D131678: hicpp-signed-bitwise - Return location of the operand (and not of the operator beginning)

carlosgalvezp added a comment. On the other hand, if the `0xFF` becomes `0xFFu`, then Clang performs an integer promotion to `unsigned int` instead of `int`, and that's why clang-tidy no longer complains. I'm not sure that's correct behavior, however: If int can represent the entire range

[PATCH] D131678: hicpp-signed-bitwise - Return location of the operand (and not of the operator beginning)

carlosgalvezp added a comment. Hi @vladimir.plyashkun ! I looked at the screenshot in the link you posted. What is the type of `value`? Consider that if `value` is an `uint8_t`, as per the integer promotion rules, it will be promoted to (signed) `int` before running the bitwise operation.

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9ae5896d9673: [clang-tidy] Add

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

carlosgalvezp updated this revision to Diff 451760. carlosgalvezp added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 Files:

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

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

[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

[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. @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] 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 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] 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] 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 added a comment. @LegalizeAdulthood I've addressed your comments, thanks for the clear instructions! One thing I didn't manage to do is build the target `docs-clang-tools-html`, it says it doesn't exist. I've enabled `LLVM_BUILD_DOCS=ON` in the CMake call - do you know if I need

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

carlosgalvezp updated this revision to Diff 439974. carlosgalvezp added a comment. Fix test name Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 Files:

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

carlosgalvezp updated this revision to Diff 439973. carlosgalvezp added a comment. Rebase and fix directory structure for doc and test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 Files:

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

carlosgalvezp added a comment. Kind 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

[PATCH] D127446: [clang-tidy] Add `-verify-config` command line argument

carlosgalvezp added a comment. This sounds like great functionality, surely saving a lot of headaches! Any reason why we wouldn't want this active by default? I'd personally even go one step further and make it hard errors - warnings are easy to miss and ignore - but I can see how it can be

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

carlosgalvezp updated this revision to Diff 435476. carlosgalvezp added a comment. Remove copy-paste comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 Files:

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.h:29 + void check(const ast_matchers::MatchFinder::MatchResult ) override; +}; + Eugene.Zelenko wrote: > Please add

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

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

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

carlosgalvezp added a comment. Fixed comments 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] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

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

[PATCH] D126891: [clang-tidy] The check should ignore final classes

carlosgalvezp added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:170 + ` involving + `final` classes. The check will not diagnose `final` marked classes, since + those cannot be used as base classes, consequently they can not violate the

[PATCH] D126891: [clang-tidy] The check should ignore final classes

carlosgalvezp added a comment. Oh, I see the unit test now, indeed `Base` does not have a virtual destructor. LGTM then! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126891/new/ https://reviews.llvm.org/D126891

[PATCH] D126891: [clang-tidy] The check should ignore final classes

carlosgalvezp added a comment. In D126891#3555456 , @steakhal wrote: > In D126891#3554039 , @carlosgalvezp > wrote: > >> Hmm, `MostDerived` **does** have a public virtual destructor in your example >> already -

[PATCH] D126891: [clang-tidy] The check should ignore final classes

carlosgalvezp added a comment. Hmm, `MostDerived` **does** have a public virtual destructor in your example already - if the base class destructor is virtual, the child class destructor is virtual. In that sense the check should not warn. Seems like there's some deeper problem in the check?

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

carlosgalvezp updated this revision to Diff 433757. carlosgalvezp added a comment. Move logic from check to registerMatchers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 Files:

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

carlosgalvezp updated this revision to Diff 433743. carlosgalvezp added a comment. Fix FIXME Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126880/new/ https://reviews.llvm.org/D126880 Files:

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

carlosgalvezp created this revision. Herald added subscribers: shchenz, kbarton, xazax.hun, mgorny, nemanjai. Herald added a project: All. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Flags uses of

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

carlosgalvezp added a comment. I tried to download the diff and apply it from the root of llvm-project, but I must be doing something wrong... $ git apply D117522.diff D117522.diff:808: trailing whitespace. - attempt to free an illegal D117522.diff:809:

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

carlosgalvezp added a comment. In D113422#748 , @njames93 wrote: > Was there any real use case for making this polymorphic, The overhead for a > virtual function call seems a little unnecessary IMO? Nothing other than readability, `CachedGlobList`

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

carlosgalvezp abandoned this revision. carlosgalvezp added a comment. Understood, thanks a lot for the clarification and for the time taken! I will then abandon this patch given the high risk involved. I will forward this feedback to MISRA in case there is a possibility to publish the upcoming

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

[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

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

carlosgalvezp added a comment. Since this is a `modernize` check, shouldn't it be proposing to use `enum class` instead of `enum`? This will conflict with `Enum.3` from `cppcoreguidelines`, and I don't think it's unreasonable that people enable both `modernize*` and `cppcoreguidelines*`. Not

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

carlosgalvezp added a comment. In D118104#3278493 , @JesApp wrote: > Anyone have an idea why it's still building? Very strange, I have no clue :/ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118104/new/ https://reviews.llvm.org/D118104

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:163 def run_tidy(args, tmpdir, build_path, queue, lock, failed_files): """Takes filenames out of queue and runs clang-tidy on them.""" LegalizeAdulthood

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

carlosgalvezp added a comment. Perhaps the patch was applied on a too old baseline, could you try rebasing on top of latest main? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118104/new/ https://reviews.llvm.org/D118104 ___ cfe-commits

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

carlosgalvezp added a comment. Looks good to me! A general comment not related to this patch is that the `get_tidy_invocation` is very error prone when called like that, since all arguments are strings. It would be better to specify the arguments: get_tidy_invocation(name="",

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:257-265 invocation = [args.clang_tidy_binary, '-list-checks'] if args.allow_enabling_alpha_checkers: invocation.append('-allow-enabling-analyzer-alpha-checkers')

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

carlosgalvezp accepted this revision. carlosgalvezp added a comment. Looks great! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116085/new/ https://reviews.llvm.org/D116085 ___ cfe-commits mailing list

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGeb3f20e8fa4b: [clang-tidy] Remove gsl::at suggestion from cppcoreguidelines-pro-bounds… (authored by carlosgalvezp). Repository: rG LLVM Github

[PATCH] D117205: [clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

carlosgalvezp abandoned this revision. carlosgalvezp added a comment. Abandoned in favor of https://reviews.llvm.org/D117857 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117205/new/ https://reviews.llvm.org/D117205

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

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

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

carlosgalvezp updated this revision to Diff 402333. carlosgalvezp marked 2 inline comments as done. carlosgalvezp edited the summary of this revision. carlosgalvezp added a comment. Fixed review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D7982: [clang-tidy] Add readability-duplicate-include check

carlosgalvezp added a comment. In D7982#3263920 , @LegalizeAdulthood wrote: > In D7982#3263790 , @carlosgalvezp > wrote: > >> Great patch, thanks!! > > Can you mark the patch as accepted in phabricator? > Thanks I

[PATCH] D7982: [clang-tidy] Add readability-duplicate-include check

carlosgalvezp accepted this revision. carlosgalvezp added a comment. Great patch, thanks!! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D7982/new/ https://reviews.llvm.org/D7982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D7982: [clang-tidy] Add readability-duplicate-include check

carlosgalvezp requested changes to this revision. carlosgalvezp added inline comments. This revision now requires changes to proceed. Comment at: clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h:1 +#if

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:347 +NoLint.SpecifiesChecks = true; + } +} Nit: tab with spaces Comment at:

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

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

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

carlosgalvezp updated this revision to Diff 401880. carlosgalvezp added a comment. Fix alphabetical order in release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117857/new/ https://reviews.llvm.org/D117857 Files:

[PATCH] D117205: [clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

carlosgalvezp added a comment. I believe this other solution has lower complexity and still achieves my goal, let me know which one you prefer! https://reviews.llvm.org/D117857 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117205/new/

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

carlosgalvezp created this revision. Herald added subscribers: shchenz, arphaman, kbarton, kristof.beyls, xazax.hun, nemanjai. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Currently the fix hint is hardcoded

[PATCH] D7982: [clang-tidy] Add readability-duplicate-include check

carlosgalvezp accepted this revision. carlosgalvezp added a comment. LGTM! Had some nits that can be fixed without review. Can't see anything else major that needs change. As said I'm fairly new here so probably a more experienced reviewer can find some more improvements/optimizations,

[PATCH] D7982: [clang-tidy] Add readability-duplicate-include check

carlosgalvezp added a comment. There's also a negative vote from @alexfh , what about that? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D7982/new/ https://reviews.llvm.org/D7982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D7982: [clang-tidy] Add readability-duplicate-include check

carlosgalvezp added a comment. Looks good! I need to familiarize myself better with the `Loc` manipulation code to be able to review the free-standing function but otherwise looks reasonable. Comment at: clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp:22

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

[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

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

[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

[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

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

[PATCH] D117205: [clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp:84 + +if (!GslHeader.empty() && (FixHint == "gsl::at()")) { Diag << FixItHint::CreateInsertion(BaseRange.getBegin(), "gsl::at(")

[PATCH] D116750: [clang][lex] Keep references to `DirectoryLookup` objects up-to-date

carlosgalvezp added a comment. Great catch and thanks for the revert! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116750/new/ https://reviews.llvm.org/D116750 ___ cfe-commits mailing list

[PATCH] D117205: [clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

carlosgalvezp marked an inline comment as done. carlosgalvezp added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:156 +- Add new option ``cppcoreguidelines-pro-bounds-constant-array-index.FixHint`` + to allow users to specify a different fix hint than

[PATCH] D117205: [clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

carlosgalvezp updated this revision to Diff 399675. carlosgalvezp added a comment. Fix check order Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117205/new/ https://reviews.llvm.org/D117205 Files:

[PATCH] D117205: [clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

carlosgalvezp added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:156 +- Add new option ``cppcoreguidelines-pro-bounds-constant-array-index.FixHint`` + to allow users to specify a different fix hint than ``gsl::at``. Eugene.Zelenko

[PATCH] D117205: [clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

carlosgalvezp updated this revision to Diff 399674. carlosgalvezp marked an inline comment as done. carlosgalvezp added a comment. Single backticks for gsl::at Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117205/new/

[PATCH] D116750: [clang][lex] Keep references to `DirectoryLookup` objects up-to-date

carlosgalvezp added a comment. In D116750#3240680 , @jansvoboda11 wrote: > In D116750#3240363 , @carlosgalvezp > wrote: > >> Hi @jansvoboda11 ! >> >> I believe this commit is the root cause of this issue: >>

[PATCH] D116750: [clang][lex] Keep references to `DirectoryLookup` objects up-to-date

carlosgalvezp added a comment. Hi @jansvoboda11 ! I believe this commit is the root cause of this issue: https://github.com/llvm/llvm-project/issues/53161 Believe it or not, I was lucky enough to run into it. I have bisected the repo and my tests indicate that this commit is the offender. Do

[PATCH] D117205: [clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

carlosgalvezp updated this revision to Diff 399604. carlosgalvezp added a comment. Fix doc. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117205/new/ https://reviews.llvm.org/D117205 Files:

[PATCH] D117205: [clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

carlosgalvezp updated this revision to Diff 399603. carlosgalvezp added a comment. Update release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117205/new/ https://reviews.llvm.org/D117205 Files:

[PATCH] D117205: [clang-tidy] Support custom fix hint for cppcoreguidelines-pro-bounds-constant-array-index

carlosgalvezp created this revision. Herald added subscribers: shchenz, arphaman, kbarton, kristof.beyls, xazax.hun, nemanjai. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Currently the fix hint is hardcoded

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

carlosgalvezp added a comment. In D116833#3236209 , @rsmith wrote: > Thanks, this looks nice. > > I think we'll need to think carefully before changing the default here. It > seems like the choice here would depend on what token the location of the >

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

This revision was automatically updated to reflect the committed changes. Closed by commit rGc4db521cea32: [clang] Introduce support for disabling warnings in system macros (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

carlosgalvezp added a comment. In D116833#3232739 , @efriedma wrote: > I'll just note here that doing this globally is likely to have unexpected > results... consider, for example: > > #include > void f() { long x = M_PI; } > > Currently, the

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

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:442 +if (PrimaryCheckValue != Value) { + std::cout << "Alias check \"" << CheckName.str() << "\" of \"" +<< PrimaryCheckName.str() << "\""

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

carlosgalvezp updated this revision to Diff 398339. carlosgalvezp retitled this revision from "[clang][Sema] Introduce support for disabling warnings in system macros" to "[clang] Introduce support for disabling warnings in system macros". carlosgalvezp added a comment. Herald added a

[PATCH] D116833: [clang][Sema] Introduce support for disabling warnings in system macros

carlosgalvezp updated this revision to Diff 398325. carlosgalvezp added a comment. Keep 80 char limit in .td file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116833/new/ https://reviews.llvm.org/D116833 Files:

[PATCH] D116833: [clang][Sema] Introduce support for disabling warnings in system macros

carlosgalvezp added a comment. Updated according to your feedback, let me know what you think! I've run `ninja check-clang` successfully, let me know if there's any other test I should run. Personally I think this `ShowInSystemMacro` should be OFF by default (to be consistent with

<    1   2   3   4   5   6   7   8   9   >