Mordante added subscribers: erichkeane, ldionne, aaron.ballman. Mordante added a comment.
In D139986#4017467 <https://reviews.llvm.org/D139986#4017467>, @dblaikie wrote: > In D139986#4008169 <https://reviews.llvm.org/D139986#4008169>, @Mordante > wrote: > >> In D139986#4005997 <https://reviews.llvm.org/D139986#4005997>, @dblaikie >> wrote: >> >>> In D139986#4003873 <https://reviews.llvm.org/D139986#4003873>, @Mordante >>> wrote: >>> >>>> In D139986#4001180 <https://reviews.llvm.org/D139986#4001180>, @Michael137 >>>> wrote: >>>> >>>>> Missed couple of test cases in libcxx >>>>> About to fix those >>>> >>>> There were more breakage due to this patch, which I fixed in D140272 >>>> <https://reviews.llvm.org/D140272>. >>>> >>>> Next time please don't commit patches when the pre-commit CI is red. This >>>> build https://buildkite.com/llvm-project/libcxx-ci/builds/16118 shows the >>>> patch will break libc++. These pre-commit CI jobs were added specifically >>>> to aid the Clang developers to validate whether their changes break >>>> libc++. Libc++ heavily relies on its pre-commit CI so breaking the CI has >>>> a huge impact. > > > >>>> Note that when libc++ breaks there might be other projects that use the >>>> latest Clang HEAD that will be affected too. (Not likely with diagnostics, >>>> but likely when the modular build fails.) >>>> >>>> When you have issues resolving the libc++ issues you can always reach out >>>> to us for assistance. >>> >>> Perhaps these LibC++ tests shouldn't be testing/using clang warnings? >>> (could the warnings be turned off in the libc++ tests?) >> >> In the failing tests we want to make sure we marked a struct as >> `[[deprecated]]`. > > At least looking at > https://reviews.llvm.org/rG54d7c4dc870c5822df3d5ce538ad3936ac6405fe - that > could be made more robust to clang changes by perhaps using `// > expected-warning {{deprecated}}` - unlikely that the word deprecated will be > removed from a deprecation warning, or that we'll emit a deprecation warning > on some unrelated line from the usage. This would make the test more > resilient to phrasing/spelling changes without, imho, significantly reducing > the functionality of the test. > > Though even more generally, I could see a test being written to use `-Werror` > and various single uses of deprecated entities, checking that they're > rejected - though that'd probably slow things down too much (singel process > invocations for each deprecated entity) - so I'd say the above > (`{{deprecated}}`) is probably a fairly good balance of making the tests less > brittle while keeping them fast/functional. > > (for `locale.convenience` for instance - I think that test could be > simplified to name each type separately, rather than using them together - so > they're on separate lines. Or maybe you can separate them onto different > lines despite the nesting, splitting the outer templtae name over multiple > lines) I don't see how that helps to validate whether we have the expected error or an error due to ill-formed code. For example just forgetting a semicolon. >> In other tests we validate error messages to make sure things are ill-formed >> as mandated. We test the diagnostic to make sure the compilation fails for >> the expected reason and not a different compilation error. Do you have a >> suggestion how we can do that without checking Clang diagnostics? >> >> In general I consider it bad practice to commit code when the CI is red. > > My take on this is that, at least to my understanding, libc++ implemented > this CI without discussiong/buy-in from other LLVM subprojects that libc++ > depends on - that I'm not sure were/are willing to accept this extra > constraint on their development. It's not suitable to bring up a CI in that > way and then say "make sure you don't break this" - we didn't agree not to > break it. Especially with tests like these that could/should be written > differently to be less brittle. Perhaps I've misunderstood this & there is > more explicit buy-in from Clang developers/owners about this relationship > between the two subprojects? > > Generally prior to this it was up to libc++ folks to cleanup after upstream > project's breakage (like lldb still does, to some extent) - though buildbots, > etc, do provide llvm/clang contributors some chance to proactively provide > fixes. This CI has been discussed with members of the Clang community. In the past I've spoken with @aaron.ballman. I know @erichkeane requested @ldionne to create this CI recently. (I wasn't present during this meeting.) I don't know whether or not the creation of this CI has been further communicated in the Clang community. Note that this CI is a benefit for Clang too. Some patches that break libc++, also break code in other external projects. This patch probably doesn't break external projects, but @erichkeane's D126907 <https://reviews.llvm.org/D126907> was reverted several times due to breakage in external projects. I also discovered some issues with that patch in libc++, so there having libc++ as test codebase would have been helpful. I'm not asking Clang developers to fix libc++. But I ask not to commit code which is known to break other subprojects without giving that subproject a headsup. In D139986#4017468 <https://reviews.llvm.org/D139986#4017468>, @dblaikie wrote: > Oh, and I meant to mention > https://buildkite.com/llvm-project/libcxx-ci/builds/16118#01851abb-f13e-4977-91df-b4274402b65a > doesn't seem, at least to my cursory reading, to show diagnostic checking > failures, but crashes? Is there something broken in the UI that's meant to > show the diagnostic mismatches you ended up fixing? (or is the link out of > date at this point, and the original failures are no longer visible? or > something else?) When I look at that output for C++2b I see error: 'warning' diagnostics expected but not seen: File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 19: 'codecvt_utf8<wchar_t, 1114111, 0>' is deprecated File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 20: 'codecvt_utf16<wchar_t, 1114111, 0>' is deprecated File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 21: 'codecvt_utf8_utf16<wchar_t, 1114111, 0>' is deprecated error: 'warning' diagnostics seen but not expected: File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 19: 'codecvt_utf8<wchar_t>' is deprecated File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 20: 'codecvt_utf16<wchar_t>' is deprecated File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 21: 'codecvt_utf8_utf16<wchar_t>' is deprecated For the modular build I also see ICEs, which are compiler bugs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139986/new/ https://reviews.llvm.org/D139986 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
