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

JesApp added a comment. Thank you! :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118104/new/ https://reviews.llvm.org/D118104 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

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

aaron.ballman closed this revision. aaron.ballman added a comment. I've commit on your behalf in 0851970af5771f6721c29d066c88a72db823ba83 , thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118104/new/ https:/

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

JesApp added a comment. No problem. :) Jesko Appelfeller, je...@appelfeller.eu please. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118104/new/ https://reviews.llvm.org/D118104 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:/

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

aaron.ballman added a comment. In D118104#3303665 , @JesApp wrote: > Thanks for accepting. Do I need to do anything to get this merged or is this > basically done? Oops, sorry for failing to ask if you needed this committed on your behalf. :-) What nam

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

JesApp added a comment. Thanks for accepting. Do I need to do anything to get this merged or is this basically done? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118104/new/ https://reviews.llvm.org/D118104 ___ cfe-commits mailing list cfe-

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In D118104#3297315 , @LegalizeAdulthood wrote: > In D118104#3296467 , > @salman-javed-nz wrot

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

JesApp added a comment. I'm not so much opposed to writing a test case for the one change I made (especially after @salman-javed-nz suggestion) as I am opposed to learning the build system. I've tried to set for a few minutes and oh man, that's a lot of targets... So yeah, If someone could mer

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

LegalizeAdulthood added a comment. In D118104#3295876 , @JesApp wrote: > In D118104#3294883 , > @LegalizeAdulthood wrote: > >> What is the feasibility of Python unit tests Instead of a heavy-weight `lit` >> orie

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

LegalizeAdulthood added a comment. In D118104#3296467 , @salman-javed-nz wrote: > Anyway, I don't want what was a drive-by comment by me baloon into a lot of > extra work for the author, so I will not push for it, unless others really > want it. I thi

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

salman-javed-nz added a comment. My bad. Test should have called clang-tidy with `--checks` in one test case, and with `--config` in the second. In both cases, the disabled check should not appear in the `Enabled checks:` printout. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118104/n

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

njames93 added a comment. In D118104#3296340 , @salman-javed-nz wrote: > In D118104#3292862 , @JesApp wrote: > >> Well, since this was more of source of confusion than actual incorrect >> behaviour, I don't thin

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

salman-javed-nz added a comment. In D118104#3292862 , @JesApp wrote: > Well, since this was more of source of confusion than actual incorrect > behaviour, I don't think there should be a test for it. > > In general though, I think the script is complex e

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

JesApp added a comment. In D118104#3294883 , @LegalizeAdulthood wrote: > What is the feasibility of Python unit tests Instead of a heavy-weight `lit` > oriented test? I don't know the code-base well enough to answer that. Is python used for testing pu

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

LegalizeAdulthood added a comment. In D118104#3292862 , @JesApp wrote: > In general though, I think the script is complex enough to warrant some > testing. What is the feasibility of Python unit tests Instead of a heavy-weight `lit` oriented test? CH

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

JesApp added a comment. Well, since this was more of source of confusion than actual incorrect behaviour, I don't think there should be a test for it. In general though, I think the script is complex enough to warrant some testing. That being said: I don't think they should be part of this patc

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

salman-javed-nz added a comment. Do you reckon it's worth expanding the test: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp to look at this issue? There isn't much testing of the run-clang-tidy.py at the moment, but it doesn't

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

JesApp added a comment. In D118104#3279654 , @njames93 wrote: > In D118104#3279524 , @JesApp wrote: > >> I can (re)start a build manually. Let's see how that goes, before we start >> doing workarounds like that.

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

njames93 added a comment. In D118104#3279524 , @JesApp wrote: > I can (re)start a build manually. Let's see how that goes, before we start > doing workarounds like that. I think there's just some serious latency problems with the pre merge checks atm

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

JesApp added a comment. I can (re)start a build manually. Let's see how that goes, before we start doing workarounds like that. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118104/new/ https://reviews.llvm.org/D118104 ___ cfe-commits mailin

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

aaron.ballman added a comment. In D118104#3278493 , @JesApp wrote: > Anyone have an idea why it's still building? Hmmm, it looks like an infrastructure issue with the CI builder. https://buildkite.com/organizations/llvm-project/pipelines/diff-checks/bui

[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

JesApp added a comment. Anyone have an idea why it's still building? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118104/new/ https://reviews.llvm.org/D118104 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

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

JesApp updated this revision to Diff 403541. JesApp marked 2 inline comments as done. JesApp added a comment. Resubmitting the same changes after rebasing onto current main. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118104/new/ https://reviews.llvm.org/D118104 Files: clang-tools-

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

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

LegalizeAdulthood 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.""" Am I missing some

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

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

JesApp added a comment. I'm new to phabricator, buildkite, your whole setup. Any idea why the patch fails? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118104/new/ https://reviews.llvm.org/D118104 ___ cfe-commits mailing list cfe-commits@li

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

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

JesApp updated this revision to Diff 403244. JesApp added a comment. As requested by @carlosgalvezp, this update uses the get_tidy_invocation function, rather than building it's own test invocation. I chose to pass all arguments of the script, in case any of them ever have an impact on what is

[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] D118104: Make run-clang-tidy.py print the configured checks correctly

JesApp added a comment. See this github issue for the bug this fixes. Repository: rG LLVM Github Monorepo 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

JesApp created this revision. JesApp added a reviewer: alexfh. JesApp added a project: clang-tools-extra. Herald added a subscriber: carlosgalvezp. JesApp requested review of this revision. Herald added a subscriber: cfe-commits. The test invocation at the start of run-clang-tidy.py (line 257) pri