Skip to content

Attributes sometimes dropped by parser#498

Closed
let-def wants to merge 3 commits intoocaml:trunkfrom
let-def:parser_attrs
Closed

Attributes sometimes dropped by parser#498
let-def wants to merge 3 commits intoocaml:trunkfrom
let-def:parser_attrs

Conversation

@let-def
Copy link
Copy Markdown
Contributor

@let-def let-def commented Mar 8, 2016

These look like mistakes introduced during refactoring.
I am not sure what is the exact syntax allowed for attributes, I just used what seemed obvious.

@alainfrisch
Copy link
Copy Markdown
Contributor

Thanks! This should probably go in 4.03. Can you rebase on that branch (I think you need to open a new PR)?

This was referenced Mar 8, 2016
@gasche
Copy link
Copy Markdown
Member

gasche commented Mar 8, 2016

One more reason to use Menhir ( #292 ) as a parser generator...

@gasche
Copy link
Copy Markdown
Member

gasche commented Mar 8, 2016

Do we know which faulty refactoring it was? Can we check that all mistakes it introduced were covered?

@Drup
Copy link
Copy Markdown
Contributor

Drup commented Mar 8, 2016

@gasche Pretty sure it was mine (#342) and menhir would help for only half of the patch. Enabling warnings is sufficient for the other half ...

@alainfrisch
Copy link
Copy Markdown
Contributor

Indeed, adding [@@@ocaml.warning "+27"] to parser.mly would have detected the errors.

@alainfrisch alainfrisch closed this Mar 8, 2016
@alainfrisch
Copy link
Copy Markdown
Contributor

We should really enable more warnings in the compiler. I've started to work on it in this branch https://github.com/ocaml/ocaml/tree/enable_more_warnings and already found one probable bug in the type-checker.

@let-def let-def deleted the parser_attrs branch August 3, 2016 16:48
lukemaurer pushed a commit to lukemaurer/ocaml that referenced this pull request Jun 25, 2021
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Sep 21, 2022
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.

4 participants