Skip to content

[ADT] Fix specialization of ValueIsPresent for PointerUnion #121847

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jan 10, 2025
Merged

[ADT] Fix specialization of ValueIsPresent for PointerUnion #121847

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
107a35d
[ADT] Make null PointerUnion with different active members compare equal
s-barannikov Jan 6, 2025
f833dff
Merge branch 'main' into adt/pointer-union-eq
s-barannikov Jan 7, 2025
13a9098
Specialize ValueIsPresent for PointerUnion
s-barannikov Jan 7, 2025
8abd5cd
Update llvm/include/llvm/ADT/PointerUnion.h
s-barannikov Jan 8, 2025
294c617
Merge branch 'main' into adt/pointer-union-eq
s-barannikov Jan 8, 2025
62157bf
Change the specialization for nullable types to cast to bool
s-barannikov Jan 8, 2025
0546ba3
Replace IsNullable with a check that the type can be converted to bool
s-barannikov Jan 9, 2025
f0140f8
Add IsNullable check back
s-barannikov Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions llvm/include/llvm/Support/Casting.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,10 +589,6 @@ template <typename To, typename From>
// ValueIsPresent
//===----------------------------------------------------------------------===//

template <typename T>
constexpr bool IsNullable =
std::is_pointer_v<T> || std::is_constructible_v<T, std::nullptr_t>;

/// ValueIsPresent provides a way to check if a value is, well, present. For
/// pointers, this is the equivalent of checking against nullptr, for Optionals
/// this is the equivalent of checking hasValue(). It also provides a method for
Expand All @@ -614,9 +610,9 @@ template <typename T> struct ValueIsPresent<std::optional<T>> {
static inline decltype(auto) unwrapValue(std::optional<T> &t) { return *t; }
};

// If something is "nullable" then we just cast it to bool to see if it exists.
// Specialization for types convertible to bool.
template <typename T>
struct ValueIsPresent<T, std::enable_if_t<IsNullable<T>>> {
struct ValueIsPresent<T, std::enable_if_t<std::is_constructible_v<bool, T>>> {
Copy link
Member

@kuhar kuhar Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep the check that T can be constructed with nullptr? There are types that can be cast to bool but are not pointer-like.

Copy link
Contributor Author

@s-barannikov s-barannikov Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it, but figured that checking whether the type is convertible to bool exactly answers the question "does an object have a value". This may not be desired for non-class types (e.g. int) but those shouldn't really be used in this context.

Copy link
Member

@kuhar kuhar Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this enable_if guards both isPresent and unwrapValue -- don't we need extra care for the latter?

Copy link
Member

@kuhar kuhar Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, I'm going to approve this as-is, use your judgement here

Copy link
Contributor Author

@s-barannikov s-barannikov Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this enable_if guards both isPresent and unwrapValue -- don't we need extra care for the latter?

Maybe, although constructibility from nullptr_t doesn't guarantee that the default implementation of unwrapValue would fit.

As I noted in the other comment, I think it would be best if we provided explicit specializations for each type like we do with simplify_type, for example. Possibly provide a generic one for T *, too. I would't commit myself to implementing this though, I've already shown my expertise in metaprogramming.

using UnwrappedType = T;
static inline bool isPresent(const T &t) { return static_cast<bool>(t); }
Copy link
Contributor

@nikic nikic Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going down this route, it would probably make sense to also change the enable_if to check std::is_convertible<T, bool> instead?

I think you could also drop the separate std::optional specialization because it also has operator bool.

Copy link
Contributor Author

@s-barannikov s-barannikov Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going down this route, it would probably make sense to also change the enable_if to check std::is_convertible<T, bool> instead?

I think it will make much more sense, will try.

I think you could also drop the separate std::optional specialization because it also has operator bool.

They are not quite the same, the specialization for std::optional derefernces the argument of unwrapValue().
This can probably be guarded by constexpr if, but personally I find the separate specialization clearer.

Copy link
Contributor Author

@s-barannikov s-barannikov Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears is_convertible only handles implicit conversions, i.e. it returns false for types with explicit operator bool(). I used is_constructible with swapped arguments instead.

static inline decltype(auto) unwrapValue(T &t) { return t; }
Expand Down
Loading