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

Also support M.[] and M.{} like M.() #6054

Closed
vicuna opened this issue Jun 27, 2013 · 4 comments
Closed

Also support M.[] and M.{} like M.() #6054

vicuna opened this issue Jun 27, 2013 · 4 comments
Milestone

Comments

@vicuna
Copy link

@vicuna vicuna commented Jun 27, 2013

Original bug ID: 6054
Reporter: kaustuv
Status: closed (set by @damiendoligez on 2014-07-16T16:41:16Z)
Resolution: fixed
Priority: low
Severity: feature
Version: 4.00.1
Target version: 4.02.0+dev
Category: ~DO NOT USE (was: OCaml general)
Tags: patch
Related to: #6635
Monitored by: @gasche kaustuv @hcarty

Bug description

I am madly in love with the M.() notation. The only way it could be better if it were also possible to write M.[] and M.{} instead of M.([]) and M.({}). Among other issues, these latter forms cause more indentation than desired in the various ocaml Emacs modes and indentation tools for the contents inside teh brackets/braces.

From a brief perusal of parser.mly, there seems to be no conflicts with adding productions beginning with mod_longident DOT to simple_expr for these cases. However, there will be duplication of code in parser.mly and possibly in the concrete syntax tree and/or in CamlP4 for these new constructs.

Also worth considering M.[| |], M.{< >}, etc. although I can't honestly remember if I've ever used these forms.

File attachments

@vicuna
Copy link
Author

@vicuna vicuna commented Jul 2, 2013

Comment author: @damiendoligez

I'm in favour, including M.[||] (doesn't make sense to have it for lists and not arrays) and M.{<>} (for completeness).

@vicuna
Copy link
Author

@vicuna vicuna commented Jan 23, 2014

Comment author: kaustuv

OK, I've updated the patch with M.[| |] and M.{< >} as well and rebased it on top of trunk as of 2014-01-23.

Note: the patch is only a proof of concept. It only extends the parser, and does not add any unit tests, nor does it touch Camlp4. It should probably be rewritten/extended to follow your coding guidelines.

@vicuna
Copy link
Author

@vicuna vicuna commented Jan 29, 2014

Comment author: @trefis

nor does it touch Camlp4.

But camlp4 now lives outside of the ocaml repository, so your patch should be merged (if people like it, of course) regardless of the status of camlp4, right?

PS: I really like the proposition!

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 4, 2014

Comment author: @gasche

Patch applied in trunk, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant