Skip to content

Conversation

@miscco
Copy link
Contributor

@miscco miscco commented Sep 27, 2019

Description

This is a work in progress of the implementation of std::span (See #4 )

Checklist:

  • I understand README.md.
  • If this is a feature addition, that feature has been voted into the C++
    Working Draft.
  • Identifiers in any product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 .
  • Identifiers in test code changes are not _Ugly.
  • Test code includes the correct headers as per the Standard, not just
    what happens to compile.
  • The STL builds and test harnesses have passed (must be manually verified
    by an STL maintainer before CI is online, leave this unchecked for initial
    submission).
  • This change introduces no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate or
    trivially copyable, etc.). If unsure, leave this box unchecked and ask a
    maintainer for help.

However, there are some troubles. I went with _Compressed_pair to implement static extent, however it seems that it is not constexpr-friendly. The tests that I have shamelessly copied from libcxx can be found here.

At this point I am unsure, which option is better:

  1. Make _Compressed_pair fully constexpr-friendly
  2. Roll a new span specific solutio similar to what the gsl did
  3. Add a general facility next to _Compressed_pair that is constexpr-friendly

Also I followed the approach of string_view and added an indirection however I an unsure whether that is really necessary

@miscco miscco requested a review from a team as a code owner September 27, 2019 09:29
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an incomplete review (we're still working on test infrastructure), but I thought I'd give you some feedback as you work on this.

@miscco
Copy link
Contributor Author

miscco commented Oct 4, 2019

@StephanTLavavej Thanks a lot. I was traveling the last week and hadnt had time to compile check my changes. I will update them most likely today.

@miscco
Copy link
Contributor Author

miscco commented Oct 4, 2019

Hm you beat me to it. I have pushed the (not tested) changes that I did while traveling. This includes implementation of iterators and using a constexpr enabled compressed pair for storage.

My appologies that you checked old code.

I did not yet implement the container constructors. What I am unsure there is whether we need the full deduction information similar to what libcxx does (see and following)

With the deduction guides at the end I would believe that we only need to ensure that for __is_span_compatible_container uses

template <class _Tp, class _ElementType>
struct __is_span_compatible_container<_Tp, _ElementType,
        void_t<
        // data(cont) and size(cont) are well formed
            decltype(data(declval<_Tp>())),
            decltype(size(declval<_Tp>())),
        // remove_pointer_t<decltype(data(cont))>(*)[] is convertible to ElementType(*)[]
            typename enable_if<
                is_convertible_v<remove_pointer_t<decltype(data(declval<_Tp &>()))>(*)[],
                                 _ElementType(*)[]>,
                nullptr_t>::type
        >>
    : public true_type {};

I that correct or do we still needto explicitely remove std::array, std::span etc from the set?

@miscco miscco force-pushed the span branch 2 times, most recently from 5fa457a to e6f36e4 Compare October 10, 2019 20:26
@miscco
Copy link
Contributor Author

miscco commented Oct 10, 2019

I have finished the implementation following the current working draft N4830.

It passes all the (expected to pass) tests from libcxx, which I have collected here.

I am not really happy about all the iterator boiler plate but meh...

@miscco miscco changed the title [WIP] <span> Implement span <span> Implement span Oct 10, 2019
Copy link
Contributor Author

@miscco miscco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed most of the comments. I have to think a bit about the others though.

BTW It is really strange that github by defaults hides a lot of the comments. Nearly missed half of them

@miscco
Copy link
Contributor Author

miscco commented Oct 18, 2019

I have addressed most of the feedback. However, due to the constraints a lot of the constructors are barely readable anymore so I have to look at that.

@miscco miscco force-pushed the span branch 2 times, most recently from 7e0a2f5 to 0543e3b Compare October 18, 2019 20:52
@miscco
Copy link
Contributor Author

miscco commented Oct 18, 2019

My kids were actually went so sleep so I finished the refactoring of _Span_iterator and _Span_storage.

I am really happy how it turned out. Thanks a lot for the suggestions.

I am still not so sure about spaceship magic

@miscco
Copy link
Contributor Author

miscco commented Oct 20, 2019

I implemented most of the remaining change requests. There are still two open points where I am unsure.

  1. Spaceship for _Span_iterator: I would await @CaseyCarter s final verdict on this one.

  2. Constraints for array constructors. I am unsure how to SFINAE out the second constraint as for constructors there is no access to the function arguments so data(_arr) is not possible. I would guess one could hard-code the return type of the array? I would love to hear some guidance on this one.

@miscco
Copy link
Contributor Author

miscco commented Jan 10, 2020

@StephanTLavavej All is fine.

I will most likely be quite unresponsive in the near future. Please proceed without me as I dont kow when I have time between changing diapers.

@StephanTLavavej
Copy link
Member

No problem, I'll take it from here. Should be able to merge this next week!

@StephanTLavavej
Copy link
Member

LWG-3369 "span's deduction-guide for built-in arrays doesn't work" has been officially filed. Also, @BillyONeal reported an LWG issue which I simultaneously encountered - to_address() isn't constexpr for user-defined iterators, which affects testing span construction from array iterators. I will push a change to implement that, unless Billy separately pushes one first. Continuing to work on the tests.

@miscco
Copy link
Contributor Author

miscco commented Jan 15, 2020

Could you elaborate on the to_address issue. As far as I can see for std::array it is constexpr Do you mean a C-array?

@StephanTLavavej
Copy link
Member

I should have clarified - it's the non-member function that span's constructor calls:

STL/stl/inc/xutility

Lines 142 to 149 in d862650

template <class _Ptr>
_NODISCARD auto to_address(const _Ptr& _Val) noexcept {
if constexpr (_Has_to_address_v<_Ptr>) {
return pointer_traits<_Ptr>::to_address(_Val);
} else {
return _STD to_address(_Val.operator->()); // plain pointer overload must come first
}
}

@miscco
Copy link
Contributor Author

miscco commented Jan 15, 2020

That is ... unfortunate :(

@CaseyCarter
Copy link
Contributor

That is ... unfortunate :(

I blame the author of WG21-P1474.

span's constructors perform _CONTAINER_DEBUG_LEVEL checking, so the
checking in _Span_extent_type was unnecessary and less specific.

_Span_extent_type's converting constructor was simply never used.
An LWG issue has been submitted; it's necessary for span testing.
@StephanTLavavej
Copy link
Member

I've finished testing span (including the _STL_VERIFY checks), and I've pushed both the constexpr to_address change and a simplification to _Span_extent_type that I found while auditing the checking. I'm now preparing a Microsoft-internal PR so we should be able to merge this very soon.

Move _Adl_verify_range out of _CONTAINER_DEBUG_LEVEL checks so that it works with checked iterators
Use _Get_unwrapped_n to check validity of iterator + size constructor
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution!

@StephanTLavavej
Copy link
Member

Waiting for the final round of Microsoft-internal testing, then I will merge this. No additional changes should be pushed at this time.

@StephanTLavavej StephanTLavavej merged commit b3598a4 into microsoft:master Jan 18, 2020
@StephanTLavavej
Copy link
Member

Success! THANK YOU for implementing this major feature and enduring this extensive code review.

fengjixuchui added a commit to fengjixuchui/STL that referenced this pull request Jan 18, 2020
@miscco
Copy link
Contributor Author

miscco commented Jan 18, 2020

Thanks a lot for all the positive feedback.

There was quite a steep learing curve, but your patient feedback helped enormously. Thanks for keeping up with it.

@miscco miscco deleted the span branch January 18, 2020 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants