Page MenuHomePhabricator

[libcxx][span] Implement P1976R2
ClosedPublic

Authored by miscco on Feb 13 2020, 12:54 PM.

Details

Summary

This resolves the NB comment about the construction of a fixed-size span from a dynamic range

Diff Detail

Event Timeline

miscco created this revision.Feb 13 2020, 12:54 PM
miscco updated this revision to Diff 244515.Feb 13 2020, 12:57 PM

Forgot the explicit for the container constructors

Harbormaster completed remote builds in B46443: Diff 244515.
miscco updated this revision to Diff 244521.Feb 13 2020, 1:11 PM

Add failing tests for explicit constructors

miscco retitled this revision from [libcxx][span] Implement P1976R1 to [libcxx][span] Implement P1976R2.Feb 14 2020, 1:38 AM

gentle ping

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 17 2020, 11:19 AM
ldionne requested changes to this revision.Mar 17 2020, 1:45 PM

I think you're missing the update of the feature-test macro? eel.is/c++draft says: #define __cpp_lib_span 202002L

libcxx/include/span
67

Could you please adjust the synopsis too?

228

Can you walk me through why we don't have a single _Container&& like in the spec of P1976R2?

This revision now requires changes to proceed.Mar 17 2020, 1:45 PM
miscco marked 2 inline comments as done.Mar 17 2020, 2:36 PM

Thanks for the review, I am unsure whether one should really update the feature test macro without the other changes in place (tuple interface, relaxed array constructors etc)

If you want to update I wouldnt mind too much though

libcxx/include/span
67

Sure will do

228

Because we do not have library concepts and ranges yet.

The _Container&& constructor you mention is the range constructor that requires ranges::contiguous_range, ranges::sized_range and ranges::borrowed_range.

miscco updated this revision to Diff 250915.Mar 17 2020, 2:45 PM

[libcxx][span] Update synopsis

miscco marked an inline comment as done.Mar 17 2020, 2:45 PM

@ldionne do you require any changes here (especially regarding the feature test macro)

ldionne accepted this revision.May 7 2020, 9:49 AM

I approved the other span changes, so the feature test macro can be updated now, right? If so, please update it and go ahead, otherwise please explain :-).

This revision is now accepted and ready to land.May 7 2020, 9:49 AM
miscco updated this revision to Diff 262932.May 8 2020, 12:42 PM
  • [libcxx][span] Update synopsis

@ldionne Thanks a lot for the review. I have added the feature test macro as I did not find it anywhere else.

Note that I do not have commit access, so you would need to commit those revisions

ldionne requested changes to this revision.May 11 2020, 5:34 AM

You need to modify the feature test macro in libcxx/utils/generate_feature_test_macro_components.py, and then generate the other files (<version>, the documentation and the tests) from it. More details in libcxx/docs/DesignDocs/FeatureTestMacros.rst.

This revision now requires changes to proceed.May 11 2020, 5:34 AM
miscco updated this revision to Diff 263379.May 12 2020, 2:01 AM
  • [libcxx][span] Update synopsis
  • Update feature test macro

@ldionne Thanks a lot for the hint. I updated the generate_feature_test_macro_components.py script and also fixed the incorrect value provided to __cpp_lib_to_array

I do not really know why there are changes to execution. Maybe someone forgot to run the script?

Can you please rebase this on top of master? I'm failing to test it locally.

miscco updated this revision to Diff 263424.May 12 2020, 6:50 AM

Rebase on master

It looks like the rebase dropped most of your changes.

That is unfortunate, i will restore them later in the evening

miscco updated this revision to Diff 263467.May 12 2020, 10:22 AM

Add changes?!

miscco updated this revision to Diff 263469.May 12 2020, 10:24 AM

second try

It seems it finally worked. No idea what went wrong as the changes were still in there

We're almost there! When you run generate_feature_test_macro_components.py, it prints out something like producing new <version> header as /var/folders/10/r6bw68bs5b9gwjtrnl9dz0vm0000gn/T/version.Xe5egJ at the end. You must mv /var/...../version.asdasd libcxx/include/version to replace the current <version> header with that one. Otherwise the __cpp_lib_span macro won't be defined properly. Currently, this causes the tests to fail with this patch applied.

Also, please exclude changes to other tests like execution.version.pass.cpp from this patch -- I'll generate them separately, I think it's an oversight.

miscco updated this revision to Diff 263606.May 12 2020, 9:54 PM

Only update spans feature test macro

ldionne accepted this revision.May 13 2020, 4:58 AM
This revision is now accepted and ready to land.May 13 2020, 4:58 AM

Thanks for the review. I do not have commit rights though.

This revision was automatically updated to reflect the committed changes.

I've updated the status page in commit 182adf120ccffe937d95d5b447f6506f82f615ec. I guess it was an oversight.

I've updated the status page in commit 182adf120ccffe937d95d5b447f6506f82f615ec. I guess it was an oversight.

Thank you!