Following on from D77085, I was never happy with the passing a mapping to the option get/store functions. This patch addresses this by using explicit specializations to handle the serializing and deserializing of enum options.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp | ||
---|---|---|
30 | As a side bonus, this originally incorrect code would cast IncludeStyle to an int, then store that value. Now it will store it as the correct enum. Any other enums passed to store will error out if there is no mapping specialization for them, forcing you to either declare the mapping or cast it to an integer type |
I like the direction of this, thank you! LGTM with a small suggestion, but you should wait a few days in case one of the other reviewers has comments.
clang-tools-extra/clang-tidy/ClangTidyCheck.h | ||
---|---|---|
30–31 | Any reason not to do: static ArrayRef<std::pair<T, StringRef>> getEnumMapping() = delete; so you get a nice error if you instantiate something without the required definition? |
Use = delete for getEnumMapping in template definition.
clang-tools-extra/clang-tidy/ClangTidyCheck.h | ||
---|---|---|
30–31 | I need to read the c++ standard, never even realised you could delete static functions like that. |
This breaks chevk-clang-tools in mac: http://45.33.8.238/mac/16292/step_8.txt
Please take a look and revert for now if it takes a while to fix.
Are you sure that this is the offending patch? I don't touch the diagnostics consumer in this patch and this is essentially NFC.
It's the only change on the blame list, things pass consistently before the change, and fail consistently after it.
I reverted this in 8f73c4432b5fa8510c99a5053c07dc70a610e1fb and check-clang-tools stopped failing.
Do you happen to have a proper build log for the failing build. That bot doesn't show what happened when building, in fact it says there was no work to do when building which seems to confuse me. My best guess for the fail is infrastructure related.
Edit: http://45.33.8.238/mac/16282/log.txt this is the log I want. It shows the failure and that this is the cause, but I'm still struggling to see what the actual cause is?????
I've relanded this with some sanity checks in 37cc4fa2eaa3d03ca8cd4947eb0d4c60e3c9b45c. If it fails again without those asserts I'll go back to the drawing board
clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp | ||
---|---|---|
31 ↗ | (On Diff #274012) | This assert is failing on the mac builds - http://45.33.8.238/mac/16309/step_8.txt yet I can see no logical reason for it |
@thakis Do those bots use gn, could that be the cause of the failures?
From what I can see gn isn't fully supported by llvm and could certainly be the cause of failing builds.
Do we have any build bots on mac that don't use gn but passed the test case?
It's possible, but unlikely, given that your change doesn't touch any build files.
…and indeed, I checked out 8f73c4432b5fa8510c99a5053c07dc70a610e1fb^ (the commit before the revert) and ran ninja -j200 check-clang-tools in a cmake build and it repros there too:
-- Build files have been written to: /Users/thakis/src/llvm-build-project [2185/2306] Linking CXX static library lib/libclangDriver.a /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: warning same member name (VE.cpp.o) in output file used for input files: tools/clang/lib/Driver/CMakeFiles/obj.clangDriver.dir/ToolChains/VE.cpp.o and: tools/clang/lib/Driver/CMakeFiles/obj.clangDriver.dir/ToolChains/Arch/VE.cpp.o (due to use of basename, truncation, blank padding or duplicate input files) [2305/2306] Running the Clang extra tools' regression tests FAIL: Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/ClangTidyDiagnosticConsumer.SortsErrors (799 of 924) ******************** TEST 'Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/ClangTidyDiagnosticConsumer.SortsErrors' FAILED ******************** Note: Google Test filter = ClangTidyDiagnosticConsumer.SortsErrors [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from ClangTidyDiagnosticConsumer [ RUN ] ClangTidyDiagnosticConsumer.SortsErrors /Users/thakis/src/llvm-project/clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp:27: Failure Expected: 2ul Which is: 2 To be equal to: Errors.size() Which is: 0 0 ClangTidyTests 0x0000000106468c65 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37 1 ClangTidyTests 0x0000000106467b36 llvm::sys::RunSignalHandlers() + 198 2 ClangTidyTests 0x0000000106469246 SignalHandler(int) + 262 3 libsystem_platform.dylib 0x00007fff6b5055fd _sigtramp + 29 4 libsystem_malloc.dylib 0x00007fff6b4c5a3d tiny_malloc_from_free_list + 555 5 ClangTidyTests 0x000000010647186f testing::Test::Run() + 527 6 ClangTidyTests 0x00000001064726e6 testing::TestInfo::Run() + 566 7 ClangTidyTests 0x0000000106472f17 testing::TestCase::Run() + 247 8 ClangTidyTests 0x000000010647b737 testing::internal::UnitTestImpl::RunAllTests() + 1287 9 ClangTidyTests 0x000000010647b1bb testing::UnitTest::Run() + 171 10 ClangTidyTests 0x000000010646a4c9 main + 121 11 libdyld.dylib 0x00007fff6b30ccc9 start + 1 ******************** ******************** Failed Tests (1): Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/ClangTidyDiagnosticConsumer.SortsErrors Testing Time: 21.55s Unsupported : 3 Passed : 918 Expectedly Failed: 2 Failed : 1
I then did the same again with the change reverted, and everything passes there.
From what I can see gn isn't fully supported by llvm and could certainly be the cause of failing builds.
Do we have any build bots on mac that don't use gn but passed the test case?
It's correct that the gn build isn't supported (not just not fully: not at all) and if this would break things only there, then that wouldn't be a reason for revert. In practice, things very rarely are broken in the GN build only, and the GN bots cycle an order of magnitude faster, so they tend to show problems before the cmake bots do.
In this case, trunk was broken for > 12h, so I'd expect there's lots of evidence of this being broken on cmake bots too. However, the mac bots aren't on buildbot but on http://green.lab.llvm.org/green/ for whatever reason, and I find that page hard to read. http://green.lab.llvm.org/green/job/clang-stage1-RA/ has a bunch of red on the LHS … ah yeah http://green.lab.llvm.org/green/job/clang-stage1-RA/11966/console has " Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/ClangTidyDiagnosticConsumer.SortsErrors" at the bottom.
So this showed up on the regular mac bots, yes.
The tree's been broken for over half a day to this. I'd suggest taking breakage supports a bit more serious going forward, instead of replying with "it can't possibly be this patch" and "it must be bot setup weirdness" going forward :)
Thanks for clarifying about the regular mac bots. Is this isolated to just mac bots?
If it is isolated then the logical reason for this failure would be a subtle bug in the host toolchain.
When I landed for a second time, I noticed that the TestCheck in ClangTidyDiagnosticsConsumerTest.cpp gets instantiated, but the registerMatchers function never gets called, this is the root cause for why the test fails.
However you just need to read the the code to see how that situation shouldn't happen.
Without a mac to test on, I wont be able to investigate further(no pre merge mac test bot also makes this significantly harder for me to test), Tried testing on older clang compiler and wasn't able to reproduce any error.
clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp | ||
---|---|---|
225 | @aaron.ballman @thakis I figured out a way to prevent the test cases failing on mac, still can't figure out the root cause. Would this be acceptable for now? As far as I can see some subtle linker bug in the default mac os linker isn't happy when this test case is included. This actual test case runs just fine, but the test in ClangTidyDiagnosticsConsumerTest doesn't run properly. It appears to instantiate the context and then instantiate the check, but it never attempts to register it, causing the whole test case to fail. |
Any reason not to do:
so you get a nice error if you instantiate something without the required definition?