[libcxx-commits] [PATCH] D129048: Rewording the "static_assert" to static assertion

Fri Jul 22 06:55:08 PDT 2022

erichkeane added a comment.

In D129048#3671657 <https://reviews.llvm.org/D129048#3671657>, @ldionne wrote:

> In D129048#3671568 <https://reviews.llvm.org/D129048#3671568>, @aaron.ballman wrote:
>
>> FWIW, I've convinced myself that I agree with you here that the burden probably should have been on libc++ maintainers in this case. libc++ almost feels more like a downstream consumer of Clang in terms of testing needs, so I think when tests break because Clang diagnostic wording or behavior changes in a standards conforming way, libc++ folks should address it, but if Clang changes in a way that's not standards conforming, then Clang folks should address it. (Roughly speaking.)
>
> Yes, exactly. It's always possible to write tests that depend on implementation details of another component in subtle ways, and the fact that such brittle tests exist doesn't create a responsibility on that component to avoid breaking those downstream users. Here, it's about Clang making a valid change and libc++ containing brittle tests, but it happens quite often where libc++ changes something valid and a downstream consumer is broken in some way. The LLVM revert culture has some benefits to keep things working in a post-commit CI world, however I've noticed that it also creates a lot of friction between projects and sometimes makes important work much more difficult to land than it should. For example, we're currently in the middle of D128146 <https://reviews.llvm.org/D128146> where LLDB reverted an important patch for LLVM 15 because one of their tests broke. This is not something that comes up super often, but it's extremely disruptive and frustrating when it does, and I think it would be worth trying to address. Anyway, I'm digressing now, but I'll try to talk to a few people at the LLVM Dev Meeting to see if this is a shared concern and to think about potential solutions to address this.
>
>> That said, I absolutely think we all need to continue to collaborate closely with one another as best we can when issues arise, and I really appreciate the discussion on how we can do that!
>
> Agreed.
>
>> For this particular issue, I'd like @Codesbyusman to continue to try to fix the libc++ testing issues (it's good experience), but if that takes significantly longer (say, more than 8 hours of his effort), perhaps @ldionne or someone else from libc++ will have a moment to step in to help?
>
> Replacing `static_assert` by `(static_assert|static assertion)` should do the trick. See the patch attached to this comment, I think it should satisfy the CI @Codesbyusman.
> F23872372: static_assert.diff <https://reviews.llvm.org/F23872372>

I just want to say: I really appreciate the insight you gave into the libc++ testing in this thread.  From the CFE's perspective, our view of precommit is "it is basically always misconfigured or broken in some way", so knowing that you guys keep yours running well is nice to hear!  We'll be more cognizant in the future.

I DO think there IS a sizable difference between patches like this one, and patches like my deferred-concepts (which, btw, the libc++ tests have been great for coming up with new tests... though it was a pain to figure out how to run them in the first place!). My patches DID deserve the reverts it received so far, so don't see Aaron's mention as an issue with that.

I appreciate your suggestion/patch file, that IS a much better regex than we'd come up with, and looks like it should work.  So thank you!  We'll commit that once @Codesbyusman can get it integrated into his patch (and we'll make sure CI passes too!).

Thanks again Louis!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129048/new/

https://reviews.llvm.org/D129048