Page MenuHomePhabricator

SFINAE span default constructor on Extent == 0
ClosedPublic

Authored by ldionne on Dec 30 2019, 5:00 AM.

Details

Summary

The default constructor of a static span requires _Extent == 0 so SFINAE it out rather than using a static_assert

Diff Detail

Event Timeline

miscco created this revision.Dec 30 2019, 5:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2019, 5:00 AM
miscco updated this revision to Diff 235588.Dec 30 2019, 5:38 AM

better message

What is the benefit to the user to have this constructor SFINAE away?
With static_assert we can craft an error message.

Hm, this fixes a todo that was written by you..

No seriously I believe this is indeed a correctness issue.

First the standard marks this as a "Constraint". As far as my standardese goes this requires that this function should be SFINAEd out.

Also if we would not remove the default constructor span<T, 2> would fulfill the`default_constructible` concept although it does not.

So I believe there is no alternative to removing that constructor.

Also if we would not remove the default constructor span<T, 2> would fulfill the`default_constructible` concept although it does not.

That is a compelling argument.

Since you pointed this out, we probably need a test showing that this is true for:

  • dynamically sized spans
  • statically sized spans of size 0
  • but NOT for statically sized spans of size > 0
miscco added a comment.EditedJan 5 2020, 9:47 PM

Library concepts are not yet implemented.

I had a look at it over the weekend. The problem is that requires expressions are not yet implemented so we cannot even add those concepts yet. I didnt find out how to pass -fconcepts to gcc-9 in LIT and was too lazy to build everything on windows with MSVC.

So I would keep them as a todo?

miscco updated this revision to Diff 236609.Jan 7 2020, 9:09 AM
  • [libcxx] span: Check default constructible
miscco added a comment.Jan 7 2020, 9:09 AM

Added type_traits and concepts check for the different spans

CaseyCarter requested changes to this revision.Jan 7 2020, 9:16 AM
CaseyCarter added a subscriber: CaseyCarter.
CaseyCarter added inline comments.
libcxx/test/std/containers/views/span.cons/default.pass.cpp
17

Late comment: <span> isn't guaranteed to export __cpp_lib_concepts; this should include <version> before checking __cpp_lib_concepts.

This revision now requires changes to proceed.Jan 7 2020, 9:16 AM
miscco updated this revision to Diff 236616.Jan 7 2020, 9:29 AM
  • [libcxx] span: Check default constructible
miscco marked 2 inline comments as done.Jan 7 2020, 9:29 AM
miscco added inline comments.
libcxx/test/std/containers/views/span.cons/default.pass.cpp
17

I added it one tooth-brushing with my kids ago. That feels like an eternity but if it qualifies as late...

miscco marked an inline comment as done.Jan 7 2020, 9:30 AM

Added missing include of <version>

CaseyCarter resigned from this revision.Jan 7 2020, 10:12 AM

LGTM. (Resigning as reviewer rather than approving to avoid confusing the folks who are actually authorized to approve.)

Gentle Ping

miscco updated this revision to Diff 243944.Feb 11 2020, 11:51 AM

[libcxx][span] Use requires clause for default constructor

gentle ping

EricWF requested changes to this revision.Mar 11 2020, 8:17 PM

Does this really need concepts?

libcxx/include/span
203–204

We don't use concepts anywhere else in the library. Why are you not using SFINAE instead?

What happens when you include this file when requires isn't supported?

This revision now requires changes to proceed.Mar 11 2020, 8:17 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 11 2020, 8:17 PM

You are absolutely right.

While it is technically correct as both span and concepts are a C++20 feature I belive this change is not helpful. It is just soo much better :(

That said, we should switch to concepts once the library concepts and ranges are fully available as that is needed for conformance of the range based contructors.

How do I revert an update in phabricator?

miscco updated this revision to Diff 250022.Mar 12 2020, 12:24 PM

Do not require concepts

Ping, I guess the clang format should not be applied?

miscco marked an inline comment as done.Apr 22 2020, 1:13 AM
ldionne accepted this revision.May 7 2020, 9:47 AM

All concerns by other reviewers seem to have been addressed, feel free to move forward. Don't apply clang-format changes. Thanks!

ldionne commandeered this revision.May 14 2020, 6:33 AM
ldionne edited reviewers, added: miscco; removed: ldionne.

I'm committing this, but I'm removing concepts from the tests.

Please don't require features we don't implement like that, it's just pointless. You basically have dead code in your test as it stands.

ldionne updated this revision to Diff 263988.May 14 2020, 6:34 AM

Get rid of concepts

This revision was not accepted when it landed; it landed in state Needs Review.May 14 2020, 6:58 AM
This revision was automatically updated to reflect the committed changes.