[libcxx-commits] [PATCH] D125283: reverts "[libc++] Explicitly reject `uniform_int_distribution<bool>` and `<char>`."

Wed Jun 1 22:30:23 PDT 2022

cjdb abandoned this revision.
cjdb added inline comments.

================
Comment at: libcxx/include/__random/binomial_distribution.h:28
+template <class _IntType = int>
+class _LIBCPP_TEMPLATE_VIS binomial_distribution {
 public:
----------------
ldionne wrote:
> cjdb wrote:
> > ldionne wrote:
> > > cjdb wrote:
> > > > ldionne wrote:
> > > > > Since we are removing the restriction for these distributions too, we should also be adding tests for their behavior.
> > > > I'd do so if the tests checked `short`, etc. were valid, but I don't think it's my responsibility to fill out the tests when they don't check all standard-required patterns. Each of the things I haven't touched only test `int` and one of `long` or `long long`.
> > > I think it's easier to forget about the fact that this code used to work by accident. With that in mind, what this patch effectively does is *add* a brand new extension (that we will document and support officially) for additional types in `binomial_distribution` & friends. What I'm saying is that this new extension needs to be tested, just like we test everything else in the library. Otherwise, we don't have a way to make sure that we support it properly, and that's one of the reason why we made sure that people would not use it by accident (by adding `static_assert`s).
> > > 
> > > I'm not asking that you improve the overall test coverage for the distributions on other standard-mandated types (although that would certainly be nice). Our testing of distributions is very light, and that's a shame. What I'm asking is to meet the bar that we have for all patches: that you test the new patterns that we did not officially support previously and that we will support going forward.
> > > I think it's easier to forget about the fact that this code used to work by accident. With that in mind, what this patch effectively does is *add* a brand new extension 
> > 
> > No, it's not adding a brand new extension. libc++ has always supported this, regardless of the library's intention. That's how Hyrum's Law works.
> > 
> > Both Chrome OS and Android have been severely and adversely impacted by the changes made by D114920, which is why I'm trying to revert it. We were given no warning and no window of opportunity to change anything and have had the rug pulled out from under us. I am simply trying to unbreak the scores of packages that are using this as a result of this "accident", and would appreciate libc++ taking responsibility for the problems that D114920 has caused our teams.
> > 
> > > I'm not asking that you improve the overall test coverage for the distributions on other standard-mandated types 
> > 
> > That's exactly what's being asked here. You don't have the tests for `short`, `unsigned int`, etc., and in order for me to add tests for `unsigned char` and friends, I will need to do this very thing.
> > 
> > The lack of tests for supported code is indeed a shame, and it's the responsibility of libc++ developers to ensure that libc++ code is tested. I encourage libc++ contributors to take some time out to improve the tests for `binomial_int_distribution` and friends, but again, D125283 is a rollback of a patch that breaks downstream users who were not broken prior to this patch.
> > libc++ has always supported this, regardless of the library's intention. That's how Hyrum's Law works.
> 
> It's not because something compiles that it's supported or fine to use, and I don't think it's fair to invoke Hyrum's Law to pretend the contrary here. For instance, you can do all kinds of things like use `std::sort` with a non-strict-weak-ordering and it will compile, yet it's UB and it's clearly not something support. Have people been doing it and getting away with results that work for them? Definitely, in some cases. That's exactly what I'm worried about here, and that's why I want this to be tested. I don't want to allow something to compile if we don't know that it works properly -- that wouldn't be responsible for our users. In other words, I'd rather break some code than silently let code compile yet maybe be wrong, especially since we know this has happened in the past in this very area of the code.
> 
> > it's the responsibility of libc++ developers to ensure that libc++ code is tested
> 
> With respect, this isn't a great way to engage with the project. This is an open source project, and we accept contributions based on their technical merit and their correctness. We do our very best to cater to our users needs, because that makes the project relevant. However, you seem to imply that libc++ (or its contributors) have a responsibility to answer to the needs of specific users, and I don't think that's right. ChromeOS is using something that is officially undefined in the Standard -- it's not libc++'s problem, really, and the adversarial mentality taken by this patch isn't the best way to fix things. If you didn't want to do the work, even just a comment on D114920 explaining the situation faced by ChromeOS would have been better, because we would have been in a situation to collaboratively find a solution instead of starting with the assertion that libc++ did something wrong.
> 
> I don't think it's useful to continue this thread. I went ahead and created a patch that should address the issues encountered by ChromeOS while increasing the consistency and test coverage of libc++: https://reviews.llvm.org/D126823. If you are OK with the direction taken in D126823, I think this patch can be abandoned.
> 
> Also, I was mistaken when I said this had shipped in LLVM 14 -- it has not, so we don't need to cherry-pick anything once this is fixed.
> > it's the responsibility of libc++ developers to ensure that libc++ code is tested
>
> With respect, this isn't a great way to engage with the project. This is an open source project, and we accept contributions based on their technical merit and their correctness. We do our very best to cater to our users needs, because that makes the project relevant. However, you seem to imply that libc++ (or its contributors) have a responsibility to answer to the needs of specific users, and I don't think that's right. ChromeOS is using something that is officially undefined in the Standard -- it's not libc++'s problem, really, and the adversarial mentality taken by this patch isn't the best way to fix things. If you didn't want to do the work, even just a comment on D114920 explaining the situation faced by ChromeOS would have been better, because we would have been in a situation to collaboratively find a solution instead of starting with the assertion that libc++ did something wrong.

I understand that you're not interested in continuing the discussion, and I largely agree, but I do need to set the record straight on one thing. In its full context, the nested quote is not saying "take this patch and add the tests yourselves", which is how it is being framed here. I doubt you intentionally framed it this way, but the snippet comes across as hostile in isolation nonetheless. It's also not saying that libc++ is beholden to specific users.  More explicitly, there are four key points that I was trying to communicate:

1. the tests for `binomial_int_distribution` are lacking (which sucks),
2. that adding comprehensive tests for `binomial_int_distribution<bool>`, etc., would be the same as adding comprehensive tests for `binomial_int_distribution<short>`, etc.,
3. that I believe adding comprehensive tests for `binomial_int_distribution<short>`, etc., is a responsibility for libc++ regulars, and
4. that adding such tests are beyond the scope of a revert.

In future, as requested, I'll comment on merged patches rather than proposing reverts.

> If you are OK with the direction taken in D126823, I think this patch can be abandoned.

A forward-fix is also fine. @CrystalSplitter has provided commentary on D126823, since they're directly familiar with the pain points.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125283