ned14 / outcome

Provides very lightweight outcome<T> and result<T> (non-Boost edition)
https://ned14.github.io/outcome
Other
675 stars 62 forks source link

Use std::addressof instead of operator& #276

Closed gix closed 1 year ago

gix commented 1 year ago

Noticed this while using wil, where some wrappers have custom operator& (e.g. HANDLE* unique_process_handle::operator&).

I didn't add tests for the iostream stuff since it seems broken (a contained error is written twice, once via state and a second time explicitly) and somehow excludes void value/error types (even though the implementation has void-overloads).

ned14 commented 1 year ago

Custom address returning operators are an abomination and anybody who uses them deserves what they get.

Equally, we don't control the third party code we are forced to use. Thanks for the fix.

BurningEnlightenment commented 1 year ago

Noticed this while using wil

Slightly OT: Have you taken any additional steps to integrate outcome and WIL? Last time I was in for the ride, I found the _nothrow types pretty cumbersome to use with outcome...

gix commented 1 year ago

Not sure what to do about GCC9, it fails to compile both result<udt5> k7(k6); and result<udt5, int> k10(k9);. Looks like it chooses a wrong/different constructor: explicit_valueorerror_converting_constructor_tag instead of value_converting_constructor_tag. The former has a !concepts::basic_result<T> predicate which GCC9 does not like (the concept uses typename U::value_type etc. but U is a reference type). Is there a decay missing somewhere?

Slightly OT: Have you taken any additional steps to integrate outcome and WIL?

No, I'm mostly using the unique_ptr/unique_any wrappers.

ned14 commented 1 year ago

Not sure what to do about GCC9, it fails to compile both result k7(k6); and result<udt5, int> k10(k9);. Looks like it chooses a wrong/different constructor: explicit_valueorerror_converting_constructor_tag instead of value_converting_constructor_tag. The former has a !concepts::basic_result predicate which GCC9 does not like (the concept uses typename U::value_type etc. but U is a reference type). Is there a decay missing somewhere?

GCC 9 had a pretty early concepts implementation, if it works on other compilers and on later GCCs, that's usually the reason.

If we need to add a decay to help out the concepts implementation on GCC 9, I have no objections in principle but I'd need to study it properly.

gix commented 1 year ago

Maybe it helps. From what I gathered: For this concept

  template <class U>
  concept OUTCOME_GCC6_CONCEPT_BOOL basic_result =
  OUTCOME_V2_NAMESPACE::is_basic_result<U>::value ||
  (requires(U v) { OUTCOME_V2_NAMESPACE::basic_result<typename U::value_type, typename U::error_type, typename U::no_value_policy_type>(v); } &&    //
   detail::convertible<U, OUTCOME_V2_NAMESPACE::basic_result<typename U::value_type, typename U::error_type, typename U::no_value_policy_type>> &&  //
   detail::base_of<OUTCOME_V2_NAMESPACE::basic_result<typename U::value_type, typename U::error_type, typename U::no_value_policy_type>, U>);

U is basic_result<void, std::error_code, ...>& (a reference). Asserting both parts separately, the first part (is_basic_result) is true, but the second part is invalid (so GCC9 does not short-circuit here). Commenting out the second part makes it work, so does applying a decay to U (either here or replacing !concepts::basic_result<T>) or placing the second part into another concept.

https://godbolt.org/z/YhnrdTYPo

ned14 commented 1 year ago

https://godbolt.org/z/YhnrdTYPo

Well that's a perfect demo of exactly the issue here isn't it?

Unfortunately, sprinkling decay around here isn't going to help because if you change static_assert(always_true<int&>) to static_assert(always_true<int>), it still fails to compile as int::dummy isn't valid code.

I'm going to propose that the actual best solution here is to not use legacy GCC concepts at all? So, concepts would no longer be enabled in GCCs before 10?

Thoughts?

ned14 commented 1 year ago

I disabled use of concepts on pre-GCC-10 in develop branch. Is your PR now all green?

ned14 commented 1 year ago

@gix Be aware that the next Boost release merge window closes tomorrow. If this fix isn't before then, it'll be next Boost release.