Skip to content
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

SI-8385 make sure $quasiquote$tuple gets reified properly #3611

Merged
merged 3 commits into from Mar 10, 2014

Conversation

densh
Copy link
Contributor

@densh densh commented Mar 9, 2014

Previously due to greediness of SyntacticApplied there was a chance that
quasiquote tuple placeholder got reified as its representation rather
than its meaning.

review @retronym

Previously due to greediness of SyntacticApplied there was a chance that
quasiquote tuple placeholder got reified as its representation rather
than its meaning.
@densh densh added this to the 2.11.0-RC2 milestone Mar 9, 2014
@densh densh added the tested label Mar 9, 2014
// rest will always be non-empty due to the fact that every single-parens
// application will be reified by reifyTreePlaceholder before it gets here.
case SyntacticApplied(id @ Ident(nme.QUASIQUOTE_TUPLE), first :: rest) =>
mirrorBuildCall(nme.SyntacticApplied, reifyTreePlaceholder(Apply(id, first)), reify(rest))
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to put the fix next to TuplePlaceholder, so that all placeholders are processed next to each other? Or even better - maybe it's possible to even integrate the fix into TuplePlaceholder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible to put it into reifyTreePlaceholder but not into TuplePlaceholder as it doesn't do any reification, just wrapping/unwrapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@xeno-by
Copy link
Member

xeno-by commented Mar 9, 2014

Also could we hack up other placeholders in the same way, something like q"(T1, T2)[Int]" or whatnot?

@densh
Copy link
Contributor Author

densh commented Mar 10, 2014

@xeno-by Probably not, as Apply is mostly used for encoding of term placeholders and it doesn't overlap with other trees.

@densh densh removed the tested label Mar 10, 2014
@xeno-by
Copy link
Member

xeno-by commented Mar 10, 2014

LGTM then

adriaanm added a commit that referenced this pull request Mar 10, 2014
SI-8385 make sure $quasiquote$tuple gets reified properly
@adriaanm adriaanm merged commit 7f07d44 into scala:master Mar 10, 2014
@adriaanm adriaanm modified the milestones: 2.11.0-RC3, 2.11.0-RC2 Mar 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants