dblaikie added a comment. 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) > 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. 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
