Re: gas-mode.el - Comments welcome

[Heike C. Zimmerer]

Stefan Monnier <address@hidden> writes:

> - when you define faces used for font-locking, try to make them inherit from
>   font-lock's default faces.  I personally hate color-based highlighting and
>   use font-based highlighting instead, and every mode which redefines its
>   own set of faces is just a pain in the <beep>, unless it does it by
>   inheriting from the default faces.

They lack the inherit.  Will be corrected.

> - I'd use word and symbol syntaxes instead of `gas-re-sym' and friends.
>   I.e. instead of (skip-chars-forward gas-skip-sym), I'd use
>   (skip-syntax-forward "w_") and I'd use "\\_>" and "\\_<" for symbol
>   boundaries instead of gas-re-nosym.

There's a comment at the start of gas-parse-comment-really which
explains why I prefer regexp over syntax tables.

> - it seems that comment-start, comment-start-skip, comment-end,
>   comment-end-skip are not defined.

Not sure where what they would be needed for.  I'm providing my own
fill functions.  Did I overlook anything?

> - try C-u M-x checkdoc-current-buffer RET
>   It'll complain about things which you may rightfully decide to leave
>   unchanged, but it will also complain about some cosmetic things which you
>   need to fix, e.g. the first line should have 3 semi-colons rather than 2.
>   It may also tell you to use names such as "*-flag" for boolean vars, with
>   which I completely disagree.

Yes.  Didn't do that.

> - add some ;;;###autoload cookies (at least one for the major mode).

Yes.

> - fix naming to use the `gas-' (or `asm-') prefix for vars such as
>   `line-cache'.  Provide defvars with docstring explaning what
>   they contain.

The caches are always buffer-local.  According to what I learned from
the documentation, the prefix is only required for globals.  I
sometimes even deliberately removed the prefix from symbols to
emphasize on its local binding (as here).  Anything wrong with it?

> - gas-change-comment-string seems not to be used.

The ultimate word on it is not spoken yet; changing the comment
character is not yet tested.

> - your syntax-table also accepts comments of the form //...\n although this
>   seems to be mentioned nowhere.  Is it done on purpose or is it
>   an oversight?

Purpose.  asm-mode has it too, gas accepts it.

> - not sure if I like the "use forward-sexp to jump to next occurrence of
>   highlighted symbol".  In conjunction with C-M-t for example, this is odd.
>   The intention of forwrd-sexp-function is to allow the use of elisp code
>   when the syntax-table isn't flexible enough to describe some "sexps"
>   (e.g. sexps like "begin ... end").  If you want to extend the semantics of
>   C-M-f (rather than forward-sexp which may very well be called
>   non-interactively by other pieces of Elisp code) then rebind this
>   key sequence.

The idea why I didn't bind it to a key directly is that I wanted it to
show up wherever the user has bound a syntax-based jump to.
forward-sexp seemed the nearest match.  I'm not too lucky with it, but
couldn't come up with a better key used for a similar purpose in other
programming modes.  As to transpose-sexps (C-M-t), I did not check
this at all, since I didn't get the idea why anybody would try that in
an asm context.  I'll check it to make sure it at least does no harm.

> - The docstring of gas-mode says that gas-comment calls comment-dwim, but
>   I don't see where that happens.

OK.  Will be changed (either the docstring or the binding).

>   Also its description of comment-dwim doesn't quite match my
>   understanding of it.  AFAIK it doesn't treat "comment with leading
>   white space" specially and it doesn't remove comments unless
>   provided with a C-u prefix.

For the latter, it does.  That's part of the 'dwim'.  For the former,
it means that it tries to remove only the kind of comment point is on,
leaving the others untouched.  There are different kinds of comments,
starting in different columns, thus the 'leading white space'.  I
don't know of an other programming mode which has this distinction so
I cannot say how it treats it.

> - Try to present it as a patch to asm-mode.el (this one does set
>   comment-start and friends, so the patch would make it trivial to see that
>   this part was incorrectly removed).

This would remove about all and replace it by a different thing.  It's
not by accident or incorrectly that any part has been removed.  It's
not been removed, but rewritten without it.  There are left-overs from
asm-mode in it and I tried to use its terminology where appropriate,
since that was where I once started from, but I doubt there are many.

- Heike