[PATCH] D111100: enable plugins for clang-tidy

tstellar added a comment. I've proposed a fix for the standalone builds here D120301 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added inline comments. Comment at: clang-tools-extra/test/CMakeLists.txt:91 + if(TARGET CTTestTidyModule) + list(APPEND CLANG_TOOLS_TEST_DEPS CTTestTidyModule LLVMHello) + target_include_directories(CTTestTidyModule PUBLIC BEFORE "${CLANG_TOOLS_SOURCE_DIR}")

[PATCH] D111100: enable plugins for clang-tidy

tstellar added inline comments. Comment at: clang-tools-extra/test/CMakeLists.txt:91 + if(TARGET CTTestTidyModule) + list(APPEND CLANG_TOOLS_TEST_DEPS CTTestTidyModule LLVMHello) + target_include_directories(CTTestTidyModule PUBLIC BEFORE "${CLANG_TOOLS_SOURCE_DIR}")

[PATCH] D111100: enable plugins for clang-tidy

aaron.ballman added inline comments. Comment at: clang-tools-extra/test/CMakeLists.txt:91 + if(TARGET CTTestTidyModule) + list(APPEND CLANG_TOOLS_TEST_DEPS CTTestTidyModule LLVMHello) + target_include_directories(CTTestTidyModule PUBLIC BEFORE

[PATCH] D111100: enable plugins for clang-tidy

tstellar added inline comments. Comment at: clang-tools-extra/test/CMakeLists.txt:91 + if(TARGET CTTestTidyModule) + list(APPEND CLANG_TOOLS_TEST_DEPS CTTestTidyModule LLVMHello) + target_include_directories(CTTestTidyModule PUBLIC BEFORE "${CLANG_TOOLS_SOURCE_DIR}")

[PATCH] D111100: enable plugins for clang-tidy

cristian.adam requested changes to this revision. cristian.adam added a comment. I had on Windows two uses cases that failed due to this patch. 1. MSVC2019 with `-D LLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON` FAILED: bin/CTTestTidyModule.dll cmd.exe /C "cd . && C:\tools\cmake\bin\cmake.exe -E

[PATCH] D111100: enable plugins for clang-tidy

vtjnash accepted this revision. vtjnash added a comment. Fixed by D119199 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00

[PATCH] D111100: enable plugins for clang-tidy

mgorny added a comment. In D00#3302396 , @vtjnash wrote: > It is a somewhat worthless test IMO, and might belong better in LLVM itself > (where this functionality is defined), but there does not appear to be any > other like it currently, and it

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. Ah, this looks annoying: there are apparently two flags, CLANG_PLUGIN_SUPPORT and LLVM_ENABLE_PLUGINS, but the existing build for clang uses CLANG_PLUGIN_SUPPORT to turn off the build support and LLVM_ENABLE_PLUGINS to turn off the tests (you might not have noticed

[PATCH] D111100: enable plugins for clang-tidy

thakis added a comment. This is still breaking tests on our bots: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8822909450036830977/+/u/package_clang/stdout?format=raw FAIL: Clang Tools :: clang-tidy/CTTestTidyModule.cpp (17913 of 89667) TEST

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. It is a somewhat worthless test IMO, and might belong better in LLVM itself (where this functionality is defined), but there does not appear to be any other like it currently, and it was requested by a previous reviewer. Comparing to the code in LLVMTestingSupport,

[PATCH] D111100: enable plugins for clang-tidy

mgorny reopened this revision. mgorny added a comment. This revision is now accepted and ready to land. This breaks build of clang against system-installed LLVM: CMake Error at /usr/lib/llvm/14/lib64/cmake/llvm/AddLLVM.cmake:1821 (add_dependencies): The dependency target "LLVMHello" of

[PATCH] D111100: enable plugins for clang-tidy

vtjnash closed this revision. vtjnash added a comment. Closed by rG84f137a590e7de25c4105303e5938c40566c2dfb Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/

[PATCH] D111100: enable plugins for clang-tidy

vtjnash updated this revision to Diff 405105. vtjnash added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00 Files: clang-tools-extra/clang-tidy/tool/CMakeLists.txt

[PATCH] D111100: enable plugins for clang-tidy

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM, I think this approach is worth trying. I agree with you that we're in a bit of new territory here regarding the testing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. I decided it made the most sense to me to go with option 3, so this should be ready to land again. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00

[PATCH] D111100: enable plugins for clang-tidy

vtjnash updated this revision to Diff 404993. vtjnash added a comment. - Reland "enable plugins for clang-tidy" - fixup! Reland "enable plugins for clang-tidy": Disable the test if the user has disabled support for building it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. It looks like this is probably the first time that a test was written for a PLUGIN_TOOL, outside of the docs, so we are likely in new territory here :/ There seem to be a couple of paths forward: 1. disable the PLUGIN_TOOL option in cmake if the user has disabled the

[PATCH] D111100: enable plugins for clang-tidy

phosek added a comment. In D00#3282760 , @vtjnash wrote: > Yes, please push a revert so I can look later. Do you have a link to the > buildbot configuration, so I can reproduce that? I did some debugging and it looks like this failure is due to

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. Yes, please push a revert so I can look later. Do you have a link to the buildbot configuration, so I can reproduce that? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00

[PATCH] D111100: enable plugins for clang-tidy

phosek added a comment. We started seeing CMake error after this change landed: CMake Error at cmake/modules/AddLLVM.cmake:683 (add_dependencies): The dependency target "clang-tidy-headers" of target "CTTestTidyModule" does not exist. Call Stack (most recent call first):

[PATCH] D111100: enable plugins for clang-tidy

This revision was automatically updated to reflect the committed changes. Closed by commit rG36892727e4f1: enable plugins for clang-tidy (authored by vtjnash). Changed prior to commit: https://reviews.llvm.org/D00?vs=401997=404294#toc Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D111100: enable plugins for clang-tidy

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Thanks for the fixes, this LGTM! However, if @alexfh can sign off as well as code owner, I'd appreciate it; this is a pretty big directional change and I think he should be on

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. @aaron.ballman I think I incorporated all of your feedback. Is this okay for me to merge to main? I would like to get it in before the feature branch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/

[PATCH] D111100: enable plugins for clang-tidy

vtjnash updated this revision to Diff 401997. vtjnash added a comment. address review feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00 Files: clang-tools-extra/clang-tidy/tool/CMakeLists.txt

[PATCH] D111100: enable plugins for clang-tidy

aaron.ballman added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:428 +of our new checks. + +.. code-block:: console We should also document our expectations explicitly regarding things like ABI and API stability. e.g., remind

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. Thanks! I will work on making those changes Comment at: clang-tools-extra/test/clang-tidy/CTTestTidyModule.cpp:24-25 + + //void registerMatchers(ast_matchers::MatchFinder *Finder) override; + //void check(const ast_matchers::MatchFinder::MatchResult

[PATCH] D111100: enable plugins for clang-tidy

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:392-394 + cl::Option *load_opt = cl::getRegisteredOptions().lookup("load"); + if (load_opt) +load_opt->addCategory(ClangTidyCategory);

[PATCH] D111100: enable plugins for clang-tidy

vtjnash updated this revision to Diff 392835. vtjnash added a comment. fix cmake Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00 Files: clang-tools-extra/clang-tidy/tool/CMakeLists.txt

[PATCH] D111100: enable plugins for clang-tidy

aaron.ballman added a comment. In D00#3162293 , @vtjnash wrote: > There is clearly some more work to do to get the cmake file to be correct, > but was hoping to check this looked like the direction you thought looked > right for adding this test,

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. There is clearly some more work to do to get the cmake file to be correct, but was hoping to check this looked like the direction you thought looked right for adding this test, since there isn't an obvious example to follow. Repository: rG LLVM Github Monorepo

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. This is probably just a draft, but please let me know what you think Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00 ___ cfe-commits

[PATCH] D111100: enable plugins for clang-tidy

vtjnash updated this revision to Diff 389658. vtjnash added a comment. enable help Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00 Files: clang-tools-extra/clang-tidy/tool/CMakeLists.txt

[PATCH] D111100: enable plugins for clang-tidy

vtjnash updated this revision to Diff 389656. vtjnash added a comment. oops Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00 Files: clang-tools-extra/clang-tidy/tool/CMakeLists.txt

[PATCH] D111100: enable plugins for clang-tidy

vtjnash updated this revision to Diff 389655. vtjnash added a comment. oops Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00 Files: clang-tools-extra/docs/ReleaseNotes.rst

[PATCH] D111100: enable plugins for clang-tidy

vtjnash updated this revision to Diff 389653. vtjnash added a comment. add release notes, docs mention, and test example Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00 Files:

[PATCH] D111100: enable plugins for clang-tidy

vtjnash updated this revision to Diff 389652. vtjnash added a comment. Herald added a subscriber: arphaman. add release notes, docs mention, and test example Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. Yes, this header does everything Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00 ___ cfe-commits mailing list

[PATCH] D111100: enable plugins for clang-tidy

whisperity added a comment. Wait... Is the diff //really// supposed to be this small? You didn't include code... that one header takes care of **everything**? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/

[PATCH] D111100: enable plugins for clang-tidy

aaron.ballman edited reviewers, added: aaron.ballman, whisperity, hokein; removed: sergio.martins, iamsergio, chgans, nocnokneo, kfunk, alexfh_, segoon. aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. Herald added a

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. bump? tagging some more people who seemed like possible reviewers. I realized my original list of candidates might have mostly been inactive people. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. bump? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D111100: enable plugins for clang-tidy

vtjnash added a comment. bump? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D00/new/ https://reviews.llvm.org/D00 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D111100: enable plugins for clang-tidy

vtjnash created this revision. Herald added a subscriber: mgorny. vtjnash requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Fixes 32739 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D00 Files: