This is an archive of the discontinued LLVM Phabricator instance.

Implement P0298R3: `std::byte`
ClosedPublic

Authored by mclow.lists on Mar 15 2017, 8:23 PM.

Details

Reviewers
EricWF
rsmith
Summary

Implement std::byte from the paper http://wg21.link/P0298R3.

Split the implementation across two files; putting the stuff that needs enable_if into <type_traits> and the rest in <cstddef>, where it belongs. Sadly, this means that <cstddef> now includes <type_traits> (at the end).

Diff Detail

Event Timeline

mclow.lists created this revision.Mar 15 2017, 8:23 PM
mclow.lists added inline comments.
www/cxx1z_status.html
144

Note: the reason for the churn here is that I sorted them by paper number.
The only significant change in this file is the addition of P0298R3 and P0298R3.

Put the std::byte type in the unadorned namespace std, rather than the versioned one. This matches the behavior of other types that the compiler "knows about", like initializer_list, type_info, bad_alloc and so on.

Thanks to @rsmith for the catch.

mclow.lists marked an inline comment as done.Mar 20 2017, 1:17 PM
EricWF edited edge metadata.Mar 22 2017, 6:31 PM

I really dislike the circular dependencies between <cstddef> and <type_traits>. I suspect we can avoid this by only including <stddef.h> in <type_traits>, which would allow <cstddef> to depend on <type_traits>. @mclow.lists Does that sound good to you?

EricWF accepted this revision.Mar 23 2017, 8:52 PM

I still think we can come up with a better include structure, but we have plenty of time to figure that out before the next release.
I see no reason to hold this up.

@mclow.lists is there a feature test macro we should be implementing?

This revision is now accepted and ready to land.Mar 23 2017, 8:52 PM
EricWF added inline comments.Mar 23 2017, 9:25 PM
test/std/language.support/support.types/byteops/xor.assign.pass.cpp
13

Nit. These should be // UNSUPPORTED. XFAIL is for bugs we need to fix.

mclow.lists closed this revision.Mar 23 2017, 10:58 PM

Committed as revision 298689

mehdi_amini added inline comments.
test/std/language.support/support.types/byteops/xor.assign.pass.cpp
13

I have mixed feeling about this, in some case I have a test that I want to make sure works in some conditions and does not in other. Using UNSUPPORTED will "hide" such cases if they're unexpectingly passing.

Having to duplicate the test identically in a ".failed.cpp" with a REQUIRE: identical to the UNSUPPORTED in the .pass.cpp is adding some unnecessary overhead and maintenance burden as well.

I believe this needs compiler support, too, in order to treat

namespace std { enum class byte : unsigned char {}; }

as directly having tbaa type "omnipotent char" instead of a subtype.

That is, given:

void foo(char* x, int *y) {
  x[1] = char(y[0] & 0xff);
  x[0] = char((y[0] & 0xff00) >> 8);
}

the compiler assumes that x and y might alias each-other, and thus must have two loads of y[0]. If you replace "char" with "std::byte", the same should happen, but as of now does not.

Ping. I don't think this got resolved, and I really wouldn't like to see released in this state...can you either revert this from the library, or implement the compiler support, before the 5.0 release branch?

I believe this needs compiler support, too, in order to treat

namespace std { enum class byte : unsigned char {}; }

as directly having tbaa type "omnipotent char" instead of a subtype.

That is, given:

void foo(char* x, int *y) {
  x[1] = char(y[0] & 0xff);
  x[0] = char((y[0] & 0xff00) >> 8);
}

the compiler assumes that x and y might alias each-other, and thus must have two loads of y[0]. If you replace "char" with "std::byte", the same should happen, but as of now does not.