Skip to content

Conversation

@whizzter
Copy link

The changes so far are the addition of a new P8 dialect with bitwise operators, compound assignments and a != alias for ~=

There are a also some other shorthands for the P8 dialect listed on https://www.lexaloffle.com/pico-8.php?page=manual but I didn't run into issues with those yet in my project and didn't have time yet to check how to modify the parser (I'll probably run into those later on though but could be good to have the initial parts upstreamed?)

- compound assignments
- != alias for ~=
@fstirlitz
Copy link
Owner

Not rejecting yet, but I am definitely not going to merge this as-is.

  • I am split on whether I am willing to support a proprietary dialect in the first place.
  • As admitted above, some features of the dialect are still missing; I would rather not add features that only partially work.
  • Have you verified that this is indeed how the tokenizer works (e.g. does a + = b not work)?
  • Linter complains about style, you should fix that.
  • The test cases are missing. Add some in test/scaffolding/, then run make scaffold-tests. I have been maintaining 100% coverage* for some time, and would prefer to keep it that way.

* other than in portability code

@fstirlitz fstirlitz marked this pull request as draft September 15, 2020 08:56
@whizzter
Copy link
Author

whizzter commented Sep 15, 2020

  • I am split on whether I am willing to support a proprietary dialect in the first place.

Well there's PicoLove that's an opensource impl for porting and I'm using this for a small experiment runtime (No idea how useful it'll end up but more open support wouldn't hurt?)

  • As admitted above, some features of the dialect are still missing; I would rather not add features that only partially work.

Agreed, already ran into more missing features so i could def re-submit later if you'd prefer that.

  • Have you verified that this is indeed how the tokenizer works (e.g. does a + = b not work)?

Not yet, will verify.

  • Linter complains about style, you should fix that.
  • The test cases are missing. Add some in test/scaffolding/, then run make scaffold-tests. I have been maintaining 100% coverage* for some time, and would prefer to keep it that way.

Yeah didn't notice the existence of these 2 before submitting and noticing the CI run.

@fstirlitz fstirlitz force-pushed the master branch 3 times, most recently from 24840d1 to 5015784 Compare May 31, 2021 18:19
DollarNoob added a commit to DollarNoob/Synapsploit that referenced this pull request Oct 19, 2024
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.

2 participants