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

parse doc comments and visibiliy specifier #14

Merged
merged 15 commits into from
Aug 22, 2016

Conversation

colin-kiegel
Copy link
Owner

implementation according to #13 (comment)

@killercup Seems to work just fine, but it's a delicate / complex macro. At leas a review seems adequate - and we haven't really decided yet.

@colin-kiegel
Copy link
Owner Author

PS: current behaviour is 'unexpected token', which could be classified as a bug and is fixed by this PR.

@killercup
Copy link
Collaborator

Sorry, I'm a bit out of the loop on this—disregard anything that's totally
obvious once I properly look at this.

What are you aiming for with this? Where do you want to skip doc/vis? Can
you add some tests for this change? (Did we add macro-compile tests yet to
compare the expanded tokens? If not, we should probably do that. Should be
possible with static-cond.)

Colin Kiegel notifications@github.com schrieb am So., 7. Aug. 2016 um
23:27 Uhr:

PS: current behaviour is 'unexpected token', which could be classified as
a bug and is fixed by this PR.


You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub
#14 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABOX-cX8AvO6xVm4wnClMvpw-h_8eNpks5qdk2ygaJpZM4Jek33
.

@colin-kiegel
Copy link
Owner Author

this allows public and/or annotated struct fields, by just skipping over
these tokens. The current parser errors with unexpected token in these
cases.

will add a test for this and I might try to combine these matchers to save
iterations. :-)

PS: I was thinking about testing expanded tokens too.

Pascal Hertleif notifications@github.com schrieb am Fr., 12. Aug. 2016,
14:56:

Sorry, I'm a bit out of the loop on this—disregard anything that's totally
obvious once I properly look at this.

What are you aiming for with this? Where do you want to skip doc/vis? Can
you add some tests for this change? (Did we add macro-compile tests yet to
compare the expanded tokens? If not, we should probably do that. Should be
possible with static-cond.)

Colin Kiegel notifications@github.com schrieb am So., 7. Aug. 2016 um
23:27 Uhr:

PS: current behaviour is 'unexpected token', which could be classified as
a bug and is fixed by this PR.


You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub
<
#14 (comment)
,
or mute the thread
<
https://github.com/notifications/unsubscribe-auth/AABOX-cX8AvO6xVm4wnClMvpw-h_8eNpks5qdk2ygaJpZM4Jek33

.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#14 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHVbslB5UPPGYtoOUB5NRuEyZIfghbE9ks5qfG2VgaJpZM4Jek33
.

@colin-kiegel
Copy link
Owner Author

Ok, I did this:

  • rebased to master
  • some additional refactoring / simplification
  • and last but not least a new feature: all field attributes now map to setter functions

Still work in progress (obviously)

  • Travis is failing on stable + beta (I tested only with nightly). Will have a look.
  • I am still thinking we should start with whitelisting specific attributes, to be on the safe side (IF that is possible).

PS: I splitted my changes into a history of rather small commits.

@colin-kiegel colin-kiegel changed the title skip doc comments and visibiliy specifier parse doc comments and visibiliy specifier Aug 12, 2016
@colin-kiegel
Copy link
Owner Author

I introduced whitelisting of attributes, only these will be added to setter-fns

  • #[doc = ...]
  • #[cfg(...)]
  • #[allow(...)]

If there is demand, we could add these in the future, but I guess they probably make little sense in these positions

  • #[deny(...)]
  • #[forbid(...)]
  • #[warn(...)]

All current attributes are listed here. IMO the rest of them doesn't make sense for setter-fns.
https://doc.rust-lang.org/reference.html#attributes

@colin-kiegel
Copy link
Owner Author

colin-kiegel commented Aug 13, 2016

PS: I also re-ordered most of the arguments to introduce @foo as first tokens in recursive calls (where foo = current_state_of_macro, e.g. @parse).

@colin-kiegel
Copy link
Owner Author

PPS: If we go along this road, next thing might be to parse and filter the struct-attributes and transfer them to the impl too (candidates again doc+allow, but cfg probably not needed). However I wonder if parsing struct-attributes would survive a possible transition to procedural macros 1.1. Therefore we might wait until then before we do it.

@colin-kiegel colin-kiegel force-pushed the feature/skip-doc-comments branch 2 times, most recently from d04f55a to df6bd9d Compare August 17, 2016 20:56
@colin-kiegel
Copy link
Owner Author

@killercup undid the rebase which caused the trouble - ready for review

 #[derive(PartialEq)] does not like conditional compilation
on stable rust
@colin-kiegel colin-kiegel merged commit 8882553 into master Aug 22, 2016
@colin-kiegel colin-kiegel deleted the feature/skip-doc-comments branch August 22, 2016 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants