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-8030 force symbols on presentation compiler initialization #3262

Merged
merged 1 commit into from Dec 13, 2013

Conversation

densh
Copy link
Contributor

@densh densh commented Dec 11, 2013

This pull request forces a number of built-in symbols in presentation compiler to prevent them from being entered during parsing.

The property “parsing doesn’t enter new symbols” is tested on a rich source file that contains significant number of variations of Scala syntax.

review @retronym


implicit def implicitView: Option[Int] = None
def implicitArg1(implicit i: Int) = i + 2
def implicitArg2[T: Fooable] = implicitly[Fooable[T]]
Copy link
Member

Choose a reason for hiding this comment

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

for more variations: view bounds, upper/lower bounds

@retronym
Copy link
Member

@densh You forgot to commit Test.scala it its new location.

@densh
Copy link
Contributor Author

densh commented Dec 11, 2013

@retronym Yeap, Test.scala got lost somewhere. Will amend it tomorrow.

This commit forces a number of built-in symbols in presentation
compiler to prevent them from being entered during parsing.

The property “parsing doesn’t enter new symbols” is tested on
a rich source file that contains significant number of variations
of Scala syntax.
@densh
Copy link
Contributor Author

densh commented Dec 12, 2013

Just added Test.scala, a few examples of bounds on type arguments, and descriptive comment for symbol forcing method.

@retronym
Copy link
Member

LGTM

adriaanm added a commit that referenced this pull request Dec 13, 2013
SI-8030 force symbols on presentation compiler initialization
@adriaanm adriaanm merged commit f8b56c8 into scala:master Dec 13, 2013
Set(UnitClass, BooleanClass, ByteClass,
ShortClass, IntClass, LongClass, FloatClass,
DoubleClass, NilModule, ListClass) ++ TupleClass.seq
symbols.foreach(_.initialize)
Copy link
Contributor

Choose a reason for hiding this comment

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

@densh Sorry for being late to the party, but why isn't fullyInitializeSymbol used in place of the simpler initialize? @retronym suggested using fullyInitializeSymbol in the related ticket (I'm inlining below his comment):

And I would change that to fullyInitializeSymbol to be on the safe side as you have the tuple class, module, and module class symbols to worry about.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't really matter for the parser, which only needs to refer to the symbols, and doesn't trigger any computations that force the related symbols. The test case here shows this is sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, we should update https://github.com/scala-ide/scala-ide/pull/595/files. Thanks for clarifying.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it does any harm to initialize the related symbols, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. But keeping the two aligned will allow us to remove the ad-hoc initialization logic from our side once we drop 2.10 support.

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