-
Notifications
You must be signed in to change notification settings - Fork 40
Visibility Modifiers #1094
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
Visibility Modifiers #1094
Conversation
|
@marvinborner I slightly refactored the parser w.r.t. to Could you check whether my changes so far make sense or whether you can construct an example which either:
|
|
Sidenote: I removed a couple of |
|
I'll have to try more later, but it looks good so far. |
|
There are two very annoying things:
|
| case class Info( | ||
| doc: Doc, | ||
| // we use Maybe[Unit] instead of Boolean to have position info for validation errors | ||
| isPrivate: Maybe[Unit], | ||
| isExtern: Maybe[Unit], | ||
| // TODO this should be moved from the Info to extern functions, once we changed the syntax | ||
| externCapture: Option[CaptureSet] | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also very open to changing the design of this info object. In order to report some errors on the flags, they are positioned (using Maybe[Unit] is a hack). However, an empty Info object still contains a lot of position info and thus complicates the tests etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could be more precise by having a type hierarchy of Info. This way certain modifiers could be excluded by construction.
|
I'm fine with merging this now in order to avoid conflicts with other PRs (especially #1097). Merging it now will parse |
|
@timsueberkrueb what do you think? |
|
@jiribenes is it possible to run this on the community build before merging? |
|
@b-studios I'm happy with this being merged and rebasing my changes. |
|
Merging since its not (yet) Friday :) |
This PR reorganizes parsing of modifiers on declaraionts, which are now grouped in
Info. It also adds one new modifierprivate, which is parsed, but then ignored.