Skip to content
This repository has been archived by the owner on Nov 22, 2022. It is now read-only.

Fixes #2, closes #3, better scopes #7

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Fixes #2, closes #3, better scopes #7

wants to merge 8 commits into from

Conversation

georgjz
Copy link

@georgjz georgjz commented Jun 15, 2018

I managed to fix #2 and add operator support(#3). I also changed the scopes a bit. I tried to emulate the scoping style we use in language-65asm, a package for 6502/65816 assembly.

I would further recommend removing the highlighting for the registers. When writing machine code for architecture with only a few registers(think 6502, 6809, etc.) I find the highlighted registers overwhelming; while it is okay for register-heavy architectures like ARM, MIPS, etc. I think for Gameboy development it is simply too much - but that is a personal opinion.

@tobiasvl
Copy link
Owner

Thanks so much for the contribution! I do have some questions before merging.

There seem to be some bugs/regressions related to #2 :

  • Labels seem to no longer be parsed unless they're label definitions. To reproduce, see for example:
  • jp begin
  • call wait_vblank
  • ld a, [rLCDC]
  • Macros: ROM_HEADER CART_ROM_MBC1_RAM_BAT (note the leading space, which is needed in RGBASM to disambiguate from a label definition).
  • Any other usage of labels except as declaractions of scope
  • Label definitions with too many scopes are now false positives again. This used to work somewhat, but not perfectly, so not a huge regression. To reproduce:
  • scope1.scope2.label: is highlighted as a label, but it is an illegal label
  • Label definitions with several scopes are now just lumped together as entity.name.function.label.rgbasm (including the :), while they used to be split up with the . separators classified as keyword.operator.label.scope.rgbasm. This one might very well be more correct and isn't important to me.

These are choices you've made that I'm wondering about:

  • Assembler directives (INCLUDE, SECTION, etc) are changed from keyword.other.directive.rgbasm to support.function.pseudo.rgbasm. Is this a de facto standard for assembly languages?
  • Section types (ROM0, etc) are changed from storage.modifier.section.rgbasm to constant.language.rgbasm.

This is my first Sublime/Atom style grammar, so I'm sure many of your choices make more sense than mine, but I'd love some insight on how you made the choices!

Regarding register highlighting, feel free to open a new issue on that. I do think it would be strange to not tag registers. Actually coloring them should be up to the theme. You changed their class from variable, which is probably wise since I bet all themes highlight variables.

@georgjz
Copy link
Author

georgjz commented Jun 22, 2018

I'm so sorry for my late reply, I finally got around playing Breath of the Wild(it's amazing). I updated the pull request to address your points:

  • Labels seem to no longer be parsed unless they're label definitions. To reproduce, see for example:
  • jp begin
  • call wait_vblank
  • ld a, [rLCDC]
  • Macros: ROM_HEADER CART_ROM_MBC1_RAM_BAT (note the leading space, which is needed in RGBASM to disambiguate from a label definition).

I oversaw that one. I personally prefer only to have label definitions highlighted, having inline labels highlighted is too much color. Most other assembly language grammars don't highlight inline labels. I added code that will do that, just uncomment lines 174 ~ 184. Warning: This will cause problems with label definitions. See next point.

  • Label definitions with too many scopes are now false positives again. This used to work somewhat, but not perfectly, so not a huge regression. To reproduce:

I couldn't reproduce this, but with this new commit it doesn't occur("it works on my machine"😜). If it still happens, upload your test code as a gist and tell me which syntax theme you use so I can try and reproduce this(see also point two on my scope explanation below).

BIG CATCH: If you decide to use inline label highlighting as mentioned in the point above, mind that this will again falsely highlight illegal labels. I haven't found a good way to make this work yet.

  • Label definitions with several scopes are now just lumped together as entity.name.function.label.rgbasm (including the :), while they used to be split up with the . separators classified as keyword.operator.label.scope.rgbasm. This one might very well be more correct and isn't important to me.

I fixed that, you find the new label code in lines 143 ~ 157. I personally don't think colored colons will improve readability, but that is a design choice I leave to you. If you want the single colored labels, replace the lines 143 ~ 157 with lines 158 ~ 171.

These are choices you've made that I'm wondering about:

  • Assembler directives (INCLUDE, SECTION, etc) are changed from keyword.other.directive.rgbasm to support.function.pseudo.rgbasm. Is this a de facto standard for assembly languages?
  • Section types (ROM0, etc) are changed from storage.modifier.section.rgbasm to constant.language.rgbasm.

I prefer to do it this way for two reasons:

One, I like to have assembler directives and actual CPU opcodes separated/highlighted differently. Many assembler directives do not actually produce code; commands like section or include will not produce any actual (machine) code. While other commands like if may alter your code or generate new code(like built-in assembler macros).

Two, while writing my own first assembly grammar, I was stuck for an hour with a certain regex that refused to light up. The problem wasn't the regex itself, it was the fact that not all syntax themes highlight all scopes. Simply switching the syntax theme in Atom "fixed" my problem. I found that with most syntax themes the scopes I used as replacement are more likely to be highlighted that not(if I remember correctly, it was the storage.modifier.section scope that refused to light up in the grammar I mentioned).

Another way to look at the first point is that assembler directives are specific to the assembler you use. If you were to port it to use with another assembler/toolchain, you would probably have to modify those commands and directives, while the rest of your code with CPU opcodes might only change slightly.

I hope this helps. Just ask if you need some clarifications ☺️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse labels correctly
2 participants