Skip to content

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

Merged

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki nicolasstucki requested a review from liufengyun July 30, 2018 16:17
@nicolasstucki nicolasstucki self-assigned this Jul 30, 2018
@nicolasstucki nicolasstucki force-pushed the add-postcondition-in-reify-quotes branch from fae2b09 to 60e22f8 Compare July 30, 2018 16:20
@@ -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)) {
Copy link
Contributor

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?

Copy link
Contributor Author

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)
}
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@nicolasstucki nicolasstucki force-pushed the add-postcondition-in-reify-quotes branch from 4ef1bf7 to c0e1279 Compare July 31, 2018 07:22
Copy link
Contributor

@allanrenucci allanrenucci left a 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 =>
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM

@nicolasstucki nicolasstucki merged commit cc3bec1 into scala:master Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants