This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add __pointer_int_pair
Needs RevisionPublic

Authored by philnik on Dec 18 2022, 6:58 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Summary

This is an implementation detail for move_only_function

Diff Detail

Event Timeline

philnik created this revision.Dec 18 2022, 6:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 18 2022, 6:58 PM
philnik requested review of this revision.Dec 18 2022, 6:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 18 2022, 6:58 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 483891.Dec 19 2022, 2:49 AM

Generate output

philnik updated this revision to Diff 483894.Dec 19 2022, 3:22 AM

Try to fix CI

philnik updated this revision to Diff 483915.Dec 19 2022, 4:51 AM

Try to fix CI

ldionne requested changes to this revision.Dec 19 2022, 12:04 PM
ldionne added inline comments.
libcxx/include/__math_h/log2i.h
9 ↗(On Diff #483936)

Why is this not just under __math?

23 ↗(On Diff #483936)

We might as well add a comment explaining what the function does (compute the base-2 log of an integral type)?

27 ↗(On Diff #483936)

Here and elsewhere?

libcxx/include/__utility/pointer_int_pair.h
63–65

I really feel like the parameters should be the other way around here, because this is really a pair, and we always specify both types in a pair. I suggest we do this:

enum class __bitfield_width : size_t { };

template <class _Pointer, class _IntType, __bitfield_width __int_bit_count>
struct __pointer_int_pair;

This way, we can do this, which is more obvious than what we have right now IMO:

__pointer_int_pair<Overaligned*, size_t, __bitfield_width(2)>

In particular, I don't see why we have a default size_t type for the integral type. I don't think it makes sense, since this tries to be a general pointer-and-integral-type pair.

66

I think this can be defined as a typedef inside the class?

80–85

_LIBCPP_HIDE_FROM_ABI

87

Please add a test that uses the implicit syntax here to lock in this behavior. I think it's useful to have this be non-explicit so you can do something like return {ptr, int} when you know that the return type is a __pointer_int_pair, but I think it should be locked with a test.

103–104

I would hold off creating this until you need it (which may be in the very next patch, but then we can see it in context and decide whether it pulls its weight).

110–111

I would just inline it like this. I missed the __pointer prefix when I first read it and I thought this was a bug -- I think it's a bit confusing and doesn't save a lot of typing.

116–118

Those could be constexpr, not that it wins us very much.

libcxx/test/libcxx/utilities/pointer_int_pair/constructor_assert.pass.cpp
2

Let's name this assert.constructor.pass.cpp -- I've been trying to do this consistently.

libcxx/test/libcxx/utilities/pointer_int_pair/pointer_int_pair.pass.cpp
14–17

You could use ADDITIONAL_COMPILE_FLAGS: -Wno-private-header instead.

21

Please test that you can constinit a pointer-int-pair.

23

Please test with an overaligned type and with more than 2 bits for the integer.

25

Please test with a non-std::size_t type, like bool and a few others.

This revision now requires changes to proceed.Dec 19 2022, 12:04 PM

Mainly curious, can you give an indication how much space this would save?

libcxx/include/__math_h/log2i.h
9 ↗(On Diff #483936)

Actually I think this file should be removed. Instead we should make __bit_log2 in <bit> available in all language versions.

LLVM ships the same type: PointerIntPair.

LLVM ships the same type: PointerIntPair.

This is heavily inspired from it, but we're still tweaking the API as we think makes the most sense. Since this is a private type, we can always end up changing the API if we find the we like something closer to the original one better.

ldionne added inline comments.
libcxx/include/__utility/pointer_int_pair.h
2

Note for when we come back to this review: do we need to care about endianness? This was raised by @huixie90 during the live review of D142063.