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

keep @@ attributes on exceptions #1705

Merged
merged 4 commits into from Apr 27, 2018

Conversation

Projects
None yet
4 participants
@hhugo
Copy link
Contributor

commented Apr 6, 2018

This is an early preview to gather feedback.

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

We should be able to write the following

exception E [@deprecated]
[@@deriving sexp]
@Drup

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

The original version of attribute on structure items was indeed a bit simplistic for exceptions, so I can only approve.

You can add your tests to parsing/attributes.ml (and maybe ppx/warning.ml). I don't think we have tests that exercise the typedtree iterators.

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

There is now a warning when depracated is misplaced on exceptions.
e.g exception E [@@deprecated]

File "w03.ml", line 17, characters 15-25:
Warning 53: the "deprecated" attribute cannot appear in this context

@hhugo hhugo force-pushed the hhugo:exn branch from 88804f0 to 5dc4703 Apr 9, 2018

@hhugo hhugo force-pushed the hhugo:exn branch from 5dc4703 to cdc9b9a Apr 9, 2018

@Drup

Drup approved these changes Apr 11, 2018

Copy link
Contributor

left a comment

I had a look at the new commits. The tests are comprehensive and the handling of deprecated attributes looks good!

@hhugo hhugo force-pushed the hhugo:exn branch from cdc9b9a to fe42028 Apr 11, 2018

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

Thanks for the review

@trefis

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2018

This overall looks good, but I'm wondering: why not allow attributes on local exceptions (i.e. Pexp_letexception)?

@trefis trefis self-assigned this Apr 26, 2018

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

Attributes are already allowed in local exceptions.

  let exception M [@test]
  in ()

This PR is about adding "block" level attribute ([@@test]) which does not apply here I think.
Similarly, local modules accept [@..] but not [@@..]

@trefis

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2018

Ah!

@trefis trefis merged commit dcf11ea into ocaml:trunk Apr 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hhugo hhugo deleted the hhugo:exn branch Apr 27, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented May 29, 2018

@trefis this is in the 4.07 Changes, but was not cherry-picked in 4.07. Am I correct that this should be moved to the trunk Changes?

@trefis

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

I hadn't noticed that :)

I'll let you judge (@hhugo could give his opinion as well) whether it's better move the change entry to the trunk section, or to cherry-pick the changes.
(I personally have no trouble seeing this as a bugfix, but you guys might have a different opinion)

@gasche

This comment has been minimized.

Copy link
Member

commented May 29, 2018

I don't want to change the parsetree again this late in the release cycle, so I'll leave it in trunk.

Edit: I just fixed the Changes entry placement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.