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

Switch to GNU Bison #595

Closed
3 tasks done
ISSOtm opened this issue Oct 9, 2020 · 6 comments · Fixed by #711
Closed
3 tasks done

Switch to GNU Bison #595

ISSOtm opened this issue Oct 9, 2020 · 6 comments · Fixed by #711
Labels
enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM
Milestone

Comments

@ISSOtm
Copy link
Member

ISSOtm commented Oct 9, 2020

We've been sticking to POSIX Yacc for a while, for compatibility with some target systems, especially the BSDs. However, most systems actually use GNU Bison behind the scenes, and using some of their extensions would be quite beneficial. (Note: I'm not sure if all of these are GNU extensions, if I am wrong I'll strike-through my mistakes)

This wouldn't change much for us (our CI seems to exclusively use Bison already), it'd mostly be a downstream problem, though probably not a big one. Is it worth it?

@ISSOtm ISSOtm added enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM labels Oct 9, 2020
@AntonioND
Copy link
Member

AntonioND commented Oct 10, 2020

I think this is a pretty good idea. Anything that improves error handling and messages is welcome.

EDIT: Also, I don't think that any system that has yacc doesn't also support bison.

@ISSOtm
Copy link
Member Author

ISSOtm commented Oct 10, 2020

I don't think any system cannot have Bison, but I'm pretty sure some systems don't have it by default, like the BSDs have GNU Make as a separate gmake. Similarly to using CMake, it only makes RGBDS less lightweight in terms of dependencies; I don't think it's a big deal, though, since we're not introducing anything outlandish.

If we're agreeing on the move, then should we do it in 0.4.2, or wait for 0.4.3?

@JL2210
Copy link
Member

JL2210 commented Oct 10, 2020

I think that we shouldn't change dependencies between a pre-release and an actual release, so 0.4.3 seems good.

@ISSOtm
Copy link
Member Author

ISSOtm commented Oct 10, 2020

I don't consider 0.4.2-pre an actual release, so I don't think changing deps would be a problem, but I'll respect that decision.

Further, the changelog is actually decently-sized, the lexer changes are large enough as-is, and the target release month of November is nearing, so it probably won't hurt to limit 0.4.2's scope. Marking this for 0.4.3, then.

@ISSOtm ISSOtm added this to the v0.4.3 milestone Oct 10, 2020
@ISSOtm
Copy link
Member Author

ISSOtm commented Oct 11, 2020

I have been looking at Bison's docs in preparation to the change, and one worthwhile subject to look at is which parser table type we want. I'd say IELR is best, since we mostly care about performance, but then we'd have to enable LAC to get proper error messages, which sounds slow...

It's also worth considering enabling raw token kinds, as that'd remove one level of indirection.

ISSOtm added a commit that referenced this issue Dec 9, 2020
ISSOtm added a commit that referenced this issue Dec 9, 2020
@Rangi42
Copy link
Contributor

Rangi42 commented Jan 11, 2021

Error recovery, so that RGBASM doesn't abort on the first syntax error

This could be done with:

 line		: label T_NEWLINE
 		| label cpu_command T_NEWLINE
 		| label macro T_NEWLINE
 		| label simple_pseudoop T_NEWLINE
 		| pseudoop T_NEWLINE
 		| conditional /* May not necessarily be followed by a newline, see below */
+		| error T_NEWLINE /* Continue parsing the next line on a syntax error */
 ;

It might lead to more confusing messages though, as a syntax error in one line could break many subsequent lines (like how a missing semicolon in C/C++ can give lots of unrelated-looking errors instead of one "semicolon not found").

Edit: The current label-macro-arg test errors are affected this way, by continuing a macro after a syntax error in sizeof_\1something = 1:

 ERROR: label-macro-arg.asm(38) -> label-macro-arg.asm::test_char(25):
     syntax error, unexpected '='
 while expanding symbol "VAR_DEF"
-error: Assembly aborted (1 errors)!
+ERROR: label-macro-arg.asm(38) -> label-macro-arg.asm::test_char(29):
+    Interpolated symbol "sizeof_.something" does not exist
+ERROR: label-macro-arg.asm(39) -> label-macro-arg.asm::test_char(25):
+    Label "sizeof_" created outside of a SECTION
+while expanding symbol "VAR_DEF"
+ERROR: label-macro-arg.asm(39) -> label-macro-arg.asm::test_char(25):
+    Macro "something" not defined
+ERROR: label-macro-arg.asm(39) -> label-macro-arg.asm::test_char(26):
+    'sizeof_' already defined at label-macro-arg.asm(39) -> label-macro-arg.asm::test_char(25)
+ERROR: label-macro-arg.asm(39) -> label-macro-arg.asm::test_char(26):
+    Macro "something" not defined
+ERROR: label-macro-arg.asm(39) -> label-macro-arg.asm::test_char(29):
+    Invalid format spec 'sizeof_'
+ERROR: label-macro-arg.asm(39) -> label-macro-arg.asm::test_char(29):
+    Interpolated symbol "something" does not exist
+error: Assembly aborted (8 errors)!

Rangi42 added a commit to Rangi42/rgbds that referenced this issue Jan 21, 2021
Rangi42 added a commit to Rangi42/rgbds that referenced this issue Jan 21, 2021
Rangi42 added a commit to Rangi42/rgbds that referenced this issue Jan 22, 2021
ISSOtm pushed a commit that referenced this issue Jan 22, 2021
SuperSandro2000 pushed a commit to NixOS/nixpkgs that referenced this issue Aug 7, 2021
This also changes the owner of the GitHub repository and the homepage
URL because the repository was moved to a different organisation (the
old GitHub repository now redirects to the new one).

Also, since upstream switched to GNU Bison[1], the comment about byacc
is no longer necessary.

[1] gbdev/rgbds#595
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants