skeeto / nasm-mode

Major mode for editing NASM assembly programs
The Unlicense
79 stars 17 forks source link

Modifications from @charliegreen's fork #9

Closed lassik closed 3 months ago

lassik commented 6 years ago

@charliegreen seems to have done some useful development in October 2016 at https://github.com/charliegreen/nasm-mode

It would be cool to merge the changes into mainline. @skeeto do you have time to look at them?

One bug that the changes fix for me is font-lock highlighting of 'cpu_whatever:' labels (in mainline, 'cpu' is highlighted as a keyword even though it's part of the label name).

skeeto commented 6 years ago

Thanks for all the changes, Charlie. I've cherry-picked the stuff I liked while also taking the opportunity to break up unrelated changes into smaller commits and touch things up. (I like having a nice, tidy commit history.) I left you as the author of all these commits.

I rejected your changes to the face names for two reasons. First, the Emacs Lisp reference manual specifically says not to name faces that way ("should not end in '-face'"). Second, that would needlessly break the interface for existing users. People have already customized the existing face names, including some color themes.

I also rejected adding nasm-mode to auto-mode-alist. We don't know if this is appropriate. Installing nasm-mode doesn't necessarily mean the user wants it automatically activated when opening an ".asm" file. If there was a convention for ".nasm" files then this may be appropriate for that particular extension since it's very unlikely to conflict with another file extension, but I've never seen that extension in the wild. It's up to the user to decide when and how the mode is activated.

I also made a handful of minor stylistic adjustments: tabs to spaces, docstring formatting, pushed string building work to compile time (eval-when-compile), and favoring setf to specialized functions (goto-char). The new tab-after-mnemonic behavior may eventually be customizable, e.g. an option to revert to the old behavior.

skeeto commented 6 years ago

@lassik, I think af3d935 fixes your specific problem.

lassik commented 6 years ago

Wow, that was really fast! Great work :)

@lassik, I think af3d935 fixes your specific problem.

Just confirmed that it does. It also fixes a byte-compiler warning I got before about looking-back, so everything's great :) I think MELPA ought to pick up the latest code from the master branch within a couple days. Thank you.

lassik commented 6 years ago

Hold on, this is still highlighted incorrectly with the new code: global cpu_regs. It seems to highlight the two words global cpu_ as a single directive, and regs not at all.

skeeto commented 6 years ago

Oh yeah, that makes sense as far as that specific fix was concerned. It only addressed situations with labels matching directives. Labels (symbols with trailing colons) got a higher priority than directives so that they match first. That didn't cover "global XXX" though, which still needs to be fixed.

8dcc commented 5 months ago

Hold on, this is still highlighted incorrectly with the new code: global cpu_regs. It seems to highlight the two words global cpu_ as a single directive, and regs not at all.

@lassik I think this was also fixed in #15, but please let me know if the issue persists.