Skip to content

New syntax for multiple identifiers/definitions in one declaration statement#670

Merged
rybern merged 13 commits into
stan-dev:masterfrom
rybern:array-type-syntax-multiple-decl
Feb 13, 2021
Merged

New syntax for multiple identifiers/definitions in one declaration statement#670
rybern merged 13 commits into
stan-dev:masterfrom
rybern:array-type-syntax-multiple-decl

Conversation

@rybern
Copy link
Copy Markdown
Collaborator

@rybern rybern commented Aug 10, 2020

This PR introduces new syntax for declaring and defining more than one variable with the same declaration statement, given that they have the same type.

This builds on #669 and supersedes #561.

The following program is parsed:

data {
  real x, y, z;
  array[5] int i, j;
}
transformed data {
  real a = 5, b = 6;
  real c = 5, d, e = 7;
}

Running it through --auto-format produces this equivalent program:

data {
  real x;
  real y;
  real z;
  array[5] int i;
  array[5] int j;
}
transformed data {
  real a = 5;
  real b = 6;
  real c = 5;
  real d;
  real e = 7;
}

@rybern
Copy link
Copy Markdown
Collaborator Author

rybern commented Oct 2, 2020

Could also do this for ~ statements. In the golf case study there's:
[sigma_angle, sigma_distance, sigma_y] ~ normal(0, 1);
which could be:
sigma_angle, sigma_distance, sigma_y ~ normal(0, 1);

Comment thread src/frontend/dune Outdated
(action (diff parser.messages parser_updated.messages)))
(rule
(with-stdout-to parser_updated_trimmed.messages
(run python3 %{dep:strip_redundant_parser_state.py} %{dep:parser_updated.messages})))
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seantalts what do you think about calling python3 explicitly here, instead of relying on the shebang? The issue being that my python installation isn't where the shebang points

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I used the script to update the messages for this branch and it worked great 👍

@seantalts
Copy link
Copy Markdown
Member

seantalts commented Nov 2, 2020 via email

@rybern
Copy link
Copy Markdown
Collaborator Author

rybern commented Nov 2, 2020

Ah gotcha. It's probably the most annoying part of my hipster distro. I'll revert it for now and find a workaround

I think this branch is ready for review if you get the chance, although it'll be a lot easier to see the diff once the array syntax PR is merged

@rybern rybern changed the base branch from master to cleanup November 3, 2020 19:25
@rybern rybern changed the base branch from cleanup to master November 3, 2020 19:25
@rybern
Copy link
Copy Markdown
Collaborator Author

rybern commented Nov 3, 2020

Okay, diffs are fixed now (had to work around the github bug here) so this is ready for review.

@rok-cesnovar
Copy link
Copy Markdown
Member

@rybern can you merge current master in here and we can discuss how to proceed here?

@rybern
Copy link
Copy Markdown
Collaborator Author

rybern commented Feb 11, 2021

@rok-cesnovar All merged up

@seantalts
Copy link
Copy Markdown
Member

I can take a look at this this weekend. Thank you!

@seantalts seantalts self-requested a review February 13, 2021 16:29
Copy link
Copy Markdown
Member

@seantalts seantalts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one errant comment, otherwise looks good!

I am so happy we have Menhir and scripts to properly diff and update parser.messages. Is it easier now? Too bad we can't teach github (or Menhir) to ignore e.g. changes in state machine state id numbers, lol.

Comment thread src/frontend/parser.mly Outdated
* (\* map over each variable in v (often only one), assigning each the same
* type. *\)
* let dims = Option.value dims_opt ~default:[] in
* List.map vs ~f:(fun (id, rhs_opt) -> *)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this comment and code still need to be here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, thanks!

@rybern
Copy link
Copy Markdown
Collaborator Author

rybern commented Feb 13, 2021

Thanks Sean!

Messages are definitely a bit easier with the script, yeah.

I was actually just messing around with messages again after I ran into some issues updating the tuples branch, and I discovered that menhir's gotten some nice updates: https://gitlab.inria.fr/fpottier/menhir/blob/master/CHANGES.md
We could definitely make use of the --merge function there.
I was also thinking that we could scrub the comments before committing and regenerate them when needed; might make it more readable with smaller diffs.

@rybern rybern merged commit 8357b0e into stan-dev:master Feb 13, 2021
@seantalts
Copy link
Copy Markdown
Member

The --merge-errors one, right? Are you thinking for when two people are working on the parser at the same time?

@rybern
Copy link
Copy Markdown
Collaborator Author

rybern commented Feb 17, 2021

@seantalts
Yeah --merge-errors is the main one I looked at. They have a little demo project with some message management utils that look nice: https://gitlab.inria.fr/fpottier/menhir/-/blob/master/demos/calc-syntax-errors/Makefile.messages.maintenance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants