Streamline logic related to accessor derivation in MethodSynthesis & Namers#4709
Merged
retronym merged 3 commits intoscala:2.12.xfrom Sep 8, 2015
Merged
Streamline logic related to accessor derivation in MethodSynthesis & Namers#4709retronym merged 3 commits intoscala:2.12.xfrom
retronym merged 3 commits intoscala:2.12.xfrom
Conversation
Contributor
Author
|
This is in preparation of https://github.com/adriaanm/scala/tree/traits-late-fields, which will ultimately enable the interfaces-with-default-methods encoding of traits. |
Member
There was a problem hiding this comment.
Looks like this part enters the symbols (which needs to be done before typechecking), and addDerivedTrees is called to create the corresponding trees during typechecking.
I agree it would be cleaner to keep track of the factories we create here (maybe as a attachment on the ValDef's symbol?) and avoid the duplicated logic in allValDefDerived.
Contributor
Author
There was a problem hiding this comment.
- I'll include your comment to improve on my snarky rhetoric.
Contributor
Author
|
Progress? EDIT: nope, bug in my refactoring (in |
1db8a53 to
4139bd0
Compare
Contributor
Author
|
I rebased, addressing some feedback in earlier commits, adding the assert last for easy reversal. |
Give Getter control over whether a setter is needed. For now, only mutable ValDefs entail setters. In the new trait encoding, a trait val will also receive a setter from the start. Similarly, distinguish whether to derive a field from deferredness of the val. (Later, fields will not be emitted for traits, deferred or not.)
Originally (modulo renaming & reduction of double negation in previous commit):
```
def deriveAccessors(vd: ValDef) = vd.mods.isLazy || !(
!owner.isClass
|| (vd.mods.isPrivateLocal && !vd.mods.isCaseAccessor) // this is an error -- now checking first
|| (vd.name startsWith nme.OUTER)
|| (context.unit.isJava) // pulled out to caller
|| isEnumConstant(vd)
)
def deriveAccessorTrees(vd: ValDef) = !(
(vd.mods.isPrivateLocal && !vd.mods.isLazy) // lazy was pulled out to outer disjunction
|| vd.symbol.isModuleVar // pulled out to caller
|| isEnumConstant(vd))
```
With changes in comments above, these conditions are now captured by one method.
4139bd0 to
df61ab6
Compare
Contributor
Author
|
Running community build on merge of #4723 onto this one: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/153/console |
Member
|
LGTM |
retronym
added a commit
that referenced
this pull request
Sep 8, 2015
Streamline logic related to accessor derivation in MethodSynthesis & Namers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Distinguish abstract ValDef from ValDef that needs a setter.
(Later, all trait vals will also need a setter -- right now, it's only mutable ones.)
Similarly, make explicit whether a field is needed. (Later, fields will not be emitted for traits.)
Simplify decision whether to derive accessors. Comments in the original code below (modulo renaming & reduction of double negation in previous commit) explain why I believe my changes make sense:
These conditions are now captured by one method.
Review by @lrytz, @retronym & @SethTisue. I'll check bytecode diffs (should be empty) later tonight.