[ownership] Eliminate OperandOwnershipKindMap in favor of OwnershipConstraint #34683
Conversation
|
@swift-ci test |
gottesmm
force-pushed
the
pr-d6205adb8f293c5e7c3fad8419163cc936cc4adc
branch
from
bde8a62 to
f46d5a9
Compare
|
@swift-ci test |
…nstraint. I also used this as a moment to clarify the lattice that related ValueOwnershipKind and OwnershipConstraint.
gottesmm
force-pushed
the
pr-d6205adb8f293c5e7c3fad8419163cc936cc4adc
branch
from
f46d5a9 to
a294ab6
Compare
|
@swift-ci test |
| bool Operand::isLifetimeEnding() const { | ||
| auto constraint = getOwnershipConstraint(); | ||
|
|
||
| // If we got back none, then our operand is for a type dependent operand. So |
There was a problem hiding this comment.
Clarify none means Optional::None
| // Otherwise, we may have a lifetime ending use. If we narrowed to None then | ||
| // we have a non lifetime ending use, otherwise we still have a lifetime | ||
| // ending use. If our value was not None, then obviously narrowing didn't | ||
| // happen. |
There was a problem hiding this comment.
This comment talks about "narrowing", but I don't know what's being narrowed or where it's being narrowed.
| // Otherwise, narrowing only happened if our ownership constraint did not have | ||
| // an OwnershipKind of None, but we disallow that in any case, so we can just | ||
| // unconditionally return false. | ||
| assert(constraint->getPreferredKind() != OwnershipKind::None && |
There was a problem hiding this comment.
assert(constraint->getPreferredKind() != OwnershipKind::None
I don't know how this assert is different than simply saying
constraint->isSatisfiedBy(get().getOwnershipKind())
which is always verified anyway. Why not just
// A trivial value can never have its lifetime ended.
return get().getOwnershipKind() != OwnershipKind::None
?
| "Adding kind twice to the map with different constraints?!"); | ||
| data[kindOffset] = true; | ||
| data[constraintOffset] = bool(constraint); | ||
| static OwnershipConstraint anyValueAcceptingConstraint() { |
There was a problem hiding this comment.
I have a lot of difficulty parsing names like this and attempting to sort out their meaning. It accepts any value? Obviously not. "Constraint" is already in the type name, so why is it repeated?
Whereas if it were simply any() or even unconstrained() I would immediately know what that means.
I would also know what "acceptsAnyOwnership" means, so that would be ok. It's still unnecessarily verbose because constraints always imply acceptance and Ownership is already in the name.
Starting from a minimal recognizable name, before adding any more words, the question should be "how could any reasonable person be misled by the shorter name, and how would adding more words fix that problem without creating more confusion"?
There was a problem hiding this comment.
I think that any works well.
|
I'll fix the issues you mentioned in a follow on commit. |
I also used this as a moment to clarify the lattice that related
ValueOwnershipKind and OwnershipConstraint.