Make any_internal::FastTypeId() and IdForType() constexpr #28
Conversation
This means removing all side effects from FastTypeId(). So rather than instantiate dummy_var in the first call to FastTypeId(), move this responsibility to the linker, and only read its address during execution - guaranteed to never change. This allows for more optimization opportunities, with more explicit uses of constexpr
|
Can one of the admins verify this patch? |
| // assuming the BSS segment doesn't span more than 4GiB. | ||
| template <typename Type> | ||
| struct TypeTag { | ||
| constexpr static char dummy_var = 0; |
There was a problem hiding this comment.
If Abseil supports C++17 inline variables, this should be that when available.
There was a problem hiding this comment.
We do not yet branch for C++14 and C++17, but doing so has been talked about.
| static char dummy_var; | ||
| return reinterpret_cast<size_t>(&dummy_var); | ||
| constexpr inline const void* FastTypeId() { | ||
| return &TypeTag<Type>::dummy_var; |
There was a problem hiding this comment.
If you have a constexpr std::addressof here, you'll want that to avoid ADL on the template parameter Type.
There was a problem hiding this comment.
There is no ADL here since dummy_var is always of type char, so std::addressof does not need to be used. The type Type does not matter. Side note: We do not have a portable constexpr std::addressof as it cannot be implemented without intrinsics. It might be possible for us to implement per supported configuration, but that has not yet been investigated and I suspect will only work for some.
There was a problem hiding this comment.
@calabrese-google Ah, sorry my mistake. I just saw the template parameter and reacted.
FYI here is the detection libc++ uses for __builtin_addressof.
#if !__has_builtin(__builtin_addressof) && _GNUC_VER < 700
# define _LIBCPP_HAS_NO_BUILTIN_ADDRESSOF
#endif```There was a problem hiding this comment.
Thanks. We'll likely do this at some point.
absl/types/any.h
Outdated
| size_t GetObjTypeId() const { | ||
| return obj_ == nullptr ? any_internal::FastTypeId<void>() : obj_->type_id(); | ||
| const void* GetObjTypeId() const { | ||
| return obj_ == nullptr ? any_internal::FastTypeId<void>() |
There was a problem hiding this comment.
return obj_ ? obj_->ObjTypeId() : any_internal::FastTypeId<void>()Or is the above order oddly intended?
absl/types/any.h
Outdated
| // the compiler will happily and quietly assign such a 64-bit value to a | ||
| // 32-bit integer. While a client should never do that it SHOULD still be safe, | ||
| // assuming the BSS segment doesn't span more than 4GiB. | ||
| size_t type_id() const noexcept { |
There was a problem hiding this comment.
We're casting a pointer to a integral type. I'm suspect we should use uintptr_t.
There was a problem hiding this comment.
This entire function can simply be removed as it is no longer used.
absl/types/any.h
Outdated
| #if ABSL_ANY_DETAIL_HAS_RTTI | ||
| virtual const std::type_info& Type() const noexcept = 0; | ||
| #endif // ABSL_ANY_DETAIL_HAS_RTTI | ||
|
|
||
| // Note that on 64-bit (unix) systems size_t is 64-bit while int is 32-bit and |
There was a problem hiding this comment.
Does this comment still apply? Where does int get into the mix?
|
Also, tests? |
absl/types/any.h
Outdated
| constexpr char TypeTag<Type>::dummy_var; | ||
|
|
||
| // FastTypeId<Type>() evaluates at compile/link-time to a unique pointer for the | ||
| // passed in type. These are meant to be good match for keys into maps or straight |
There was a problem hiding this comment.
Comment exceeds 80 columns.
This means removing all side effects from FastTypeId(). So rather than instantiate dummy_var in the first call to FastTypeId(), move this responsibility to the linker, and only read its address during execution - guaranteed to never change. This allows for more optimization opportunities, with more explicit uses of constexpr