-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add some post conditions to ReifyQuotes #4870
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
Add some post conditions to ReifyQuotes #4870
Conversation
fae2b09
to
60e22f8
Compare
@@ -74,6 +74,21 @@ class ReifyQuotes extends MacroTransformWithImplicits { | |||
|
|||
override def phaseName: String = "reifyQuotes" | |||
|
|||
override def checkPostCondition(tree: Tree)(implicit ctx: Context): Unit = { | |||
if (ctx.owner.ownersIterator.exists(_.isTransparentMethod)) { |
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.
Does it make sense to do these tests if ctx.compilationUnit.containsQuotesOrSplices
is false?
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, some other phase could inset by mistake one of those calls.
// assert(tree.symbol != defn.QuotedType_~) // TODO widen ~ type references at stage 0? | ||
assert(tree.symbol != defn.QuotedExpr_apply) | ||
assert(tree.symbol != defn.QuotedType_apply) | ||
} |
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.
Could this be rewritten as?
tree match {
case tree: RefTree if !ctx.inTransparentMethod =>
assert(!tree.symbol.isQuote)
// assert(!tree.symbol.isSplice) // TODO widen ~ type references at stage 0?
assert(tree.symbol != defn.QuotedExpr_~)
case tree: Select if tree.symbol == defn.QuotedExpr_~ =>
assert(Splicer.canBeSpliced(tree.qualifier))
case _ =>
}
ctx.inTransparentMethod
is more expensive than a type test
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.
Changed
4ef1bf7
to
c0e1279
Compare
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.
Otherwise LGTM
override def checkPostCondition(tree: Tree)(implicit ctx: Context): Unit = { | ||
val inTransparentMethod = ctx.owner.ownersIterator.exists(_.isTransparentMethod) | ||
tree match { | ||
case tree: RefTree if !inTransparentMethod => |
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 not use ctx.inTransparentMethod
here?
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.
No reason. I will change it
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.
LGTM
No description provided.