Page MenuHomePhabricator

[libc++] Shrink variant's index type when possible
ClosedPublic

Authored by EricWF on Nov 18 2017, 12:37 PM.

Details

Summary

Currently std::variant always uses an unsigned int to store the variant index. However this isn't nessesary and causes std::variant to be larger than it needs to be in most cases.

This patch changes the index type to be unsigned char when possible, and unsigned short or unsigned int otherwise, depending on the size (Although it's questionable if it's even possible to create a variant with 65535 elements.

Unfortunately this change is an ABI break, and as such is only enabled in ABI v2.

Diff Detail

Event Timeline

EricWF created this revision.Nov 18 2017, 12:37 PM
halyavin retitled this revision from [libc++] Shrink varient's index type when possible to [libc++] Shrink variant's index type when possible.Nov 18 2017, 12:56 PM
mpark added inline comments.Nov 18 2017, 7:39 PM
include/variant
295

s/_NumElem/_NumAlts/

300–303

Could we inline the whole thing and also avoid using the tuple utilities?
We should also add the #include <limits>

conditional_t<
  (_NumElem < numeric_limits<unsigned char>::max()),
  unsigned char,
  conditional_t<(_NumElem < numeric_limits<unsigned short>::max()),
                unsigned short,
                unsigned int>;
306

s/_IntType/_IndexType/ maybe?

test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
17

Doesn't seem like this is used?

26

Hm, I see 15 instances of Indices and 7 instances of Indexes.
Can we use Indices everywhere?

52–53

I guess I asked you to remove this above. Is this an essential part of the test?

EricWF marked 4 inline comments as done.Nov 18 2017, 7:47 PM
EricWF added inline comments.
include/variant
300–303

Not sure that's any cleaner or clearer. Plus it makes it harder to test.

Adding #include <limits> as suggested.

test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
52–53

It is a nice part of it. I pushed back on removing the function as noted above. Perhaps this can stay then?

EricWF updated this revision to Diff 123492.Nov 18 2017, 7:48 PM
  • Address inline comments.
mpark accepted this revision.Nov 18 2017, 8:18 PM
This revision is now accepted and ready to land.Nov 18 2017, 8:18 PM
EricWF closed this revision.Nov 18 2017, 8:19 PM