-
Notifications
You must be signed in to change notification settings - Fork 745
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.
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
[analysis] Implement a Lift lattice #6040
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
|
||
namespace wasm::analysis { | ||
|
||
template<Lattice L> struct Lift { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a comment on what Lift
means?
bool isBottom() const noexcept { return !this->has_value(); } | ||
bool operator==(const Element& other) const noexcept { | ||
return (isBottom() && other.isBottom()) || | ||
(!isBottom() && !other.isBottom() && **this == *other); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't **this == *other
work if both are bottom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't evaluate **this
if isBottom
because that would be dereferencing an empty optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, then the new bottom element is basically the "nothing" case of the optional? So the extra bit of the optional marks that new item, basically. That's what I was missing.
At the risk of seeming repetitive: comments 😃
|
||
template<Lattice L> struct Lift { | ||
struct Element : std::optional<typename L::Element> { | ||
bool isBottom() const noexcept { return !this->has_value(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should I read this has_value
? Do we use the empty optional as the bottom perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. I'll add a comment.
I've now improved or added doc comments to every PR in this stack. |
This lattice "lifts" another lattice by inserting a new bottom element underneath it.
This lattice "lifts" another lattice by inserting a new bottom element underneath it.
This lattice "lifts" another lattice by inserting a new bottom element
underneath it.