Record syntactic information about modifiers #1608
Conversation
| val flag = flagOfToken(in.token) | ||
| val tok = in.token | ||
| val flag = flagOfToken(tok) | ||
| val mod = atPos(in.offset) { in.nextToken(); modOfToken(tok) } |
There was a problem hiding this comment.
shorter:
val mod = atPos(in.skipToken()) { modOfToken(tok) }
Same for similar places below.
| /** Modifiers and annotations for definitions | ||
| * @param flags The set flags | ||
| object Mod { | ||
| case class Private() extends Mod |
There was a problem hiding this comment.
My hunch is that this can be done slightly more elegantly as follows:
-
Have
Modtake aflagsfield and have every case class set it. E.g.case class Protected() extends Mod(Flags.Protected)
Have a new method in Modifiers:
def | (mod: Mod) =
withFlags(mod.flags).withAddedMod(mod)
Then, only use | instead of setting flags and mods separately. I believe that would make redundant much of the current code. WDYT?
There was a problem hiding this comment.
That's a good idea, I'll do as you suggested, thanks a lot.
There was a problem hiding this comment.
@odersky I just checked the parser code, whether a semantic flag gets added depends on some compatibility check (see addFlag). But the syntactic Mod should be always added.
It's also possible to refactor the flag compatibility check to Modifiers, but the error reporting depends on compiler state (lastErrorOffset), make the refactoring difficult.
There was a problem hiding this comment.
@liufengyun I think this can be accommodated in addFlag:
if (compatible(mods.flags, flag)) mods | mod
else mods.withAddedMod(mod)
There was a problem hiding this comment.
@odersky I just pushed a commit addressing the review feedback. Note that addFlag is also used for non-syntactic flags(such as trait, method), so I added another addMod method.
And it turns out that we always need to check the added flags, thus def | (mod: Mod) = .. is not useful.
| @@ -1819,9 +1826,13 @@ object Parsers { | |||
| */ | |||
| def defOrDcl(start: Int, mods: Modifiers): Tree = in.token match { | |||
| case VAL => | |||
| patDefOrDcl(start, posMods(start, mods), in.getDocComment(start)) | |||
| val mod = atPos(in.skipToken()) { Mod.Val() } | |||
| val modPos = atPos(start) { mods.withAddedMod(mod) } | |||
There was a problem hiding this comment.
Two comments:
- Why the name
modPos? It reads like "the positon of mod" to me. Maybe usemods1instead, or inline the expression. - I don't think you need an
atPos(start)here (and neither for the other occurrences ofwithAddedMod). The position of the modifiers is set automatically to the union ofmodsandmod; so you don't need to set the position manually.
Otherwise LGTM.
There was a problem hiding this comment.
@odersky thanks for letting me know this clever trick in Positioned. I addressed them in the latest commit and rebased.
Note that there's no need to explicitly call `atPos` here, because the initializer of `Positioned` will automatically synthesize the initial position from its children elements. Refer to the definition of `Positioned` for more details.
|
Good to have this in! |
This PR defines
trait Modto record source information about modifiers.Note that
privateWithinis already present inModifiers, thus it's not repeated in Mod definitions.Review @odersky .