Skip to content

Commit

Permalink
[AliasAnalysis] Remove the default argument from getMemoryBehavior.
Browse files Browse the repository at this point in the history
ObserveRetains is not an obvious default and it does not make sense to have it
as a default argument.
  • Loading branch information
nadavrot committed Nov 23, 2015
1 parent c1dce5c commit 4b2da0a
Showing 1 changed file with 5 additions and 4 deletions.
9 changes: 5 additions & 4 deletions include/swift/SILAnalysis/AliasAnalysis.h
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,7 @@ class AliasAnalysis : public SILAnalysis {
/// TODO: When ref count behavior is separated from generic memory behavior,
/// the IgnoreRefCountIncrements flag will be unnecessary.
MemoryBehavior getMemoryBehavior(SILInstruction *Inst, SILValue V,
RetainObserveKind =
RetainObserveKind::ObserveRetains);
RetainObserveKind);

/// Returns true if Inst may read from memory in a manner that affects V.
bool mayReadFromMemory(SILInstruction *Inst, SILValue V) {
Expand Down Expand Up @@ -141,7 +140,8 @@ class AliasAnalysis : public SILAnalysis {

/// Returns true if Inst may have side effects in a manner that affects V.
bool mayHaveSideEffects(SILInstruction *Inst, SILValue V) {
MemoryBehavior B = getMemoryBehavior(Inst, V);
MemoryBehavior B = getMemoryBehavior(Inst, V,
RetainObserveKind::ObserveRetains);
return B == MemoryBehavior::MayWrite ||
B == MemoryBehavior::MayReadWrite ||
B == MemoryBehavior::MayHaveSideEffects;
Expand All @@ -151,7 +151,8 @@ class AliasAnalysis : public SILAnalysis {
/// V. This is independent of whether or not Inst may write to V and is meant
/// to encode notions such as ref count modifications.
bool mayHavePureSideEffects(SILInstruction *Inst, SILValue V) {
return getMemoryBehavior(Inst, V) == MemoryBehavior::MayHaveSideEffects;
return getMemoryBehavior(Inst, V, RetainObserveKind::ObserveRetains) ==
MemoryBehavior::MayHaveSideEffects;
}

virtual void invalidate(SILAnalysis::InvalidationKind K) { AliasCache.clear(); }
Expand Down

1 comment on commit 4b2da0a

@gottesmm
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not an obvious default? We want by default for computeMemoryBehavior to be conservative (i.e. not ignore retains) and for people to not have to reason about retains. If someone wants to do something more aggressive, they should have to think about it. Perhaps this enum has a poorly chosen name/case name?

Please sign in to comment.