<random>: Implement LWG-3519

[MattStephanson]

Fixes #2179.

Notes:

1. I've consolidated the handling of VSO-568006 (which I can't access but assume involves [[nodiscard]] and hidden friends), since it comes up much more now. I'm open to a better name, though, maybe _NODISCARD_HIDDEN_FRIEND.
2. Maintainers indicated that LWG issues were generally higher priority than bugfixes, so I applied this to subtract_with_carry_engine, even though <random>: Fixes subtract_with_carry_engine io #2088 does too. I'll work with the author of that PR on a patch to resolve the merge conflict.
3. Most of the insertion and extraction operators were just forwarding to member function _Write or _Read, so I took the liberty of inlining those.
4. piecewise_meow_distribution were a bit subtle because of (IMO) a design mistake. They contain two discrete_distribution<size_t>::param_type subobjects, one from the base class and another from their own param_type base classes. As far as I can tell, only the latter is used, so I've tried to make this more clear. (E.g., previously discrete_distribution::_Meow was a non-static member function that actually meowed its param_type argument. Now it's a member of param_type that meows the object it's called on.)

[CaseyCarter]

1. I've consolidated the handling of VSO-568006 (which I can't access but assume involves [[nodiscard]] and hidden friends), since it comes up much more now. I'm open to a better name, though, maybe _NODISCARD_HIDDEN_FRIEND.

For posterity: VSO-568006 is an EDG bug "Attributes are not accepted on friend definitions inside class templates"
Compiling:

template <class T>
struct S {
  [[xyz]] friend int f() { return 0; }
};

with EDG incorrectly diagnoses:

"test.cpp", line 3: error: attribute "xyz" does not apply here
      [[xyz]] friend int f() { return 0; }
        ^

This result is independent of the choice of attribute (xyz vs. nodiscard).

This bug has been fixed in the EDG frontend that powers IntelliSense (for which the STL has test coverage via cl /BE) but not in the EDG frontend that powers CUDA, hence our need to workaround only when __CUDACC__ is defined.

[StephanTLavavej]

Thanks! As #2088 is smaller and has been waiting for review for almost 2 months, I went ahead and reviewed it, pushed changes, and moved it to Final Review. I'd like to merge it first, and then resolve the conflict in your larger PR.

[MattStephanson]

Thanks! As #2088 is smaller and has been waiting for review for almost 2 months, I went ahead and reviewed it, pushed changes, and moved it to Final Review. I'd like to merge it first, and then resolve the conflict in your larger PR.

I went to resolve the merge conflict and found that this PR merges cleanly with main. It looks like 2088 added streaming operators with the new behavior only for subtract_with_carry_engine, while this PR applies the LWG issue to _Swc_base. Thus, there isn't a merge conflict. I can still merge in main if you'd like to see a clean test run with both changes.

[StephanTLavavej]

Thanks for checking - I don't think we need an additional test run with the latest state of main.

[StephanTLavavej]

I saw that DevCom-1555597 was reported, which appears to involve unguarded occurrences of _NODISCARD friend, like:

STL/stl/inc/exception

Line 263 in d8f03cf

_NODISCARD friend bool operator==(const exception_ptr& _Lhs, const exception_ptr& _Rhs) noexcept {

We might want to convert all existing usage to _NODISCARD_FRIEND in this PR, if that isn't too much of a distraction (however, handling it separately would also be fine).

[CaseyCarter]

We might want to convert all existing usage to _NODISCARD_FRIEND in this PR, if that isn't too much of a distraction (however, handling it separately would also be fine).

I think that should be a separate PR to avoid swamping this PR's functional change with an ocean of mechanical replacement. (Yes, "swamping with an ocean" - I'll mix all the metaphors I like!)

[MattStephanson]

I think that should be a separate PR to avoid swamping this PR's functional change [...]

I was on the fence, but if @CaseyCarter prefers a separate PR then let's do it that way.

[StephanTLavavej]

Looks great! 😻 I pushed a trivial style change and a merge with main since it had been 2 months.

I saw the minor refactorings you applied (e.g. lifting out a repeated call to size() in discrete_distribution::param_type::_Write and adding const) and they're all good. Also very happy to see operator!= guarded for C++20. I noticed pre-existing places that could be further refactored with range-for but that shouldn't become a distraction in this PR so I've recorded a todo to clean 'em up later.

[MattStephanson]

@StephanTLavavej Thanks! While I have your attention, would you mind clarifying an ABI point? I pointed out in item (4) of my first comment that I removed the discrete_distribution::_[Read|Write] functions and moved the functionality into the param_type nested class. Could you confirm that this is safe? My reasoning was that because these function have always been inline, any existing object files that need the definitions already have them. Therefore, it's unnecessary for new code to provide them. Is that right?

[StephanTLavavej]

Yep, that sounds right to me!

In general, changes to header-only functions are ABI-safe unless they do things like:

• Change object state that's stored across member function calls, in a way that old code wouldn't understand
• Change the observable behavior of functions (e.g. increasing the set of values that they can return), including when making coordinated changes between 2 or more functions
  • This is typically not an issue for Standard-mandated functions, since bugfixes will just make them do what they were already supposed to be doing.

In such cases, renaming the functions (and renaming affected internal classes, in the "changed object state" scenario) restores safety, since that transforms mix-and-match scenarios from the bad case of "old code links to new, incompatible code" to the good case of "old code has _Old functions, new code removes those and adds _New functions, they coexist happily". (With a slightly less strong safety factor, signature changes, especially arity changes, produce incompatible mangled names that are good enough as long as we aren't extern "C" and there isn't any likelihood of them changing back.)

Your moved functionality is equivalent to remove-and-add so it's safe. 😸

[CaseyCarter]

I'll go ahead and apply my single suggestion.

[StephanTLavavej]

@CaseyCarter I pushed a commit to fix test failures - and is super ultra forbidden in this codebase. (MSVC doesn't always understand it, which conveniently aligns with my personal dislike of spelling out operators 😹)

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[CaseyCarter]

and is super ultra forbidden in this codebase

Ouch, mea culpa. (and is nice since it very clearly should not be used to declare an rvalue reference. and and not are both growing on me.)

[StephanTLavavej]

Thanks for implementing this major LWG issue resolution and improving our workaround for CUDA here! 😻 🛠️ 🚀