Skip to content

parser.mly: consistently use $sloc over $loc#2031

Merged
gasche merged 1 commit intoocaml:trunkfrom
gasche:parser-all-sloc
Sep 8, 2018
Merged

parser.mly: consistently use $sloc over $loc#2031
gasche merged 1 commit intoocaml:trunkfrom
gasche:parser-all-sloc

Conversation

@gasche
Copy link
Copy Markdown
Member

@gasche gasche commented Sep 7, 2018

Menhir has two keywords to describe "the current source position",
$loc and $sloc. $sloc is more precise (it is included within $loc),
and the two differ only when the production starts with empty symbols,
and the difference only spans over whitespace.

We originally used $loc by default to emulate ocamlyacc behavior,
but $sloc is generally preferable, so this PR converts all locations
to use $sloc.

(Note: the location-of-a-symbol keyword remains $loc(...), for example
$loc($1) or $loc(foo), there is no $sloc version of those.)

I have tested this PR using #2030, and it does not, in fact, change any parse location in the compiler distribution. On the contrary, the converse change (turning all $sloc into $loc) produces a lot of location changes, as expected.

This proposed change was mentioned/discussed in #2029 and, more importantly, in #2016 (comment).

Menhir has two keywords to describe "the current source position",
$loc and $sloc. $sloc is more precise (it is included within $loc),
and the two differ only when the production starts with empty symbols,
and the difference only spans over whitespace.

We originally used $loc by default to emulate ocamlyacc behavior,
but $sloc is generally preferable, so this PR converts all locations
to use $sloc.

(Note: the location-of-a-symbol keyword remains $loc(...), for example
$loc($1) or $loc(foo), there is no $sloc version of those.)
Copy link
Copy Markdown
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

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

Looks good!

@gasche gasche merged commit cebf9c1 into ocaml:trunk Sep 8, 2018
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