GNU bug report logs - #79309
lua-mode: minor problems and suggestions

Previous Next

To reply to this bug, email your comments to 79309 AT debbugs.gnu.org.
There is no need to reopen the bug first.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Emacs Bug Report <bug-gnu-emacs <at> gnu.org>
Cc: john muhl <jm <at> pub.pink>, Juergen Hoetzel <juergen <at> hoetzel.info>
Subject: lua-mode: minor problems and suggestions
Date: Mon, 25 Aug 2025 16:52:59 +0200

Some observations about the recently added lua-mode.el:

 394   "A regexp that matches Lua builtin functions & variables.
 395 
 396 This is a compilation of 5.1, 5.2 and 5.3 builtins taken from the
 397 index of respective Lua reference manuals.")

Clearly meant as a doc string. Bracket error?

1266            (or

This `or` has only a single operand. Simple oversight or indication of something worse?

1270                (save-excursion
1271                  (and (goto-char prev-line)
1272                       ;; Check last token of previous nonblank line
1273                       (lua-last-token-continues-p)))))

`goto-char` never returns nil, but its presence as a condition here suggest that the author may have thought otherwise. (Line 996 is similar.)

2099         for type = (if (string-match-p "\\`(E" msg) :error :warning)

A regexp is overkill here; `string-prefix-p` is simpler.

1821       (while (re-search-forward "[\"'\\\t\\\n]" nil t)

This regexp is a bit muddled; too many backslashes here.
You could even rewrite the whole function using `replace-regexp-in-string`, maybe

  (concat "'"
          (replace-regexp-in-string
           (rx (or ?\" ?' ?\t ?\n ?\\))
           (lambda (s)
             (cdr (assq (aref s 0) '((?\" . "\\\"")
                                     (?\\ . "\\\\")
                                     (?\n . "\\n")
                                     (?\t . "\\t")
                                     (?'  . "\\'")))))
           str t t)
          "'"))

without any need for a temporary buffer.

Message #8 received at 79309 <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: Mattias Engdegård <mattias.engdegard <at> gmail.com>
Cc: 79309 <at> debbugs.gnu.org
Subject: Re: bug#79309: lua-mode: minor problems and suggestions
Date: Tue, 26 Aug 2025 22:34:24 -0500

[Message part 1 (text/plain, inline)]

Thanks for the review. Attached patch fixes these and a few other
little things.

Mattias Engdegård <mattias.engdegard <at> gmail.com> writes:

> Some observations about the recently added lua-mode.el:
>
>  394   "A regexp that matches Lua builtin functions & variables.
>  395 
>  396 This is a compilation of 5.1, 5.2 and 5.3 builtins taken from the
>  397 index of respective Lua reference manuals.")
>
> Clearly meant as a doc string. Bracket error?

Fixed.

> 1266            (or
>
> This `or` has only a single operand. Simple oversight or indication of something worse?

Looks like there used to be a second part but it got refactored up
into the first part and the ‘or’ just hung around.

> 1270                (save-excursion
> 1271                  (and (goto-char prev-line)
> 1272                       ;; Check last token of previous nonblank line
> 1273                       (lua-last-token-continues-p)))))
>
> `goto-char` never returns nil, but its presence as a condition
> here suggest that the author may have thought otherwise.

Removed the ‘and’ here.

> (Line 996 is similar.)

Wrapped it and the recursive call to lua-find-matching-token-word
on the next line in progn to make it clear the goto-char is for
side-effect and not return value.

> 2099         for type = (if (string-match-p "\\`(E" msg) :error :warning)
>
> A regexp is overkill here; `string-prefix-p` is simpler.

The plan is to merge the common parts between the lua modes. The
lua-ts-mode flymake backend uses string-prefix-p there so I left
this for now and it’ll get replaced with the lua-ts flymake code.

> 1821       (while (re-search-forward "[\"'\\\t\\\n]" nil t)
>
> This regexp is a bit muddled; too many backslashes here.
> You could even rewrite the whole function using `replace-regexp-in-string`, maybe
>
>   (concat "'"
>           (replace-regexp-in-string
>            (rx (or ?\" ?' ?\t ?\n ?\\))
>            (lambda (s)
>              (cdr (assq (aref s 0) '((?\" . "\\\"")
>                                      (?\\ . "\\\\")
>                                      (?\n . "\\n")
>                                      (?\t . "\\t")
>                                      (?'  . "\\'")))))
>            str t t)
>           "'"))
>
> without any need for a temporary buffer.

That works.

[0001-Various-code-cleanup-in-lua-mode-Bug-79309.patch (text/x-patch, attachment)]

Message #11 received at 79309 <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: Mattias Engdegård <mattias.engdegard <at> gmail.com>
Cc: 79309 <at> debbugs.gnu.org
Subject: Re: bug#79309: lua-mode: minor problems and suggestions
Date: Tue, 26 Aug 2025 22:35:43 -0500

[Message part 1 (text/plain, inline)]

Thanks for the review. Attached patch fixes these and a few other
little things.

Mattias Engdegård <mattias.engdegard <at> gmail.com> writes:

> Some observations about the recently added lua-mode.el:
>
>  394   "A regexp that matches Lua builtin functions & variables.
>  395 
>  396 This is a compilation of 5.1, 5.2 and 5.3 builtins taken from the
>  397 index of respective Lua reference manuals.")
>
> Clearly meant as a doc string. Bracket error?

Fixed.

> 1266            (or
>
> This `or` has only a single operand. Simple oversight or indication of something worse?

Looks like there used to be a second part but it got refactored up
into the first part and the ‘or’ just hung around.

> 1270                (save-excursion
> 1271                  (and (goto-char prev-line)
> 1272                       ;; Check last token of previous nonblank line
> 1273                       (lua-last-token-continues-p)))))
>
> `goto-char` never returns nil, but its presence as a condition
> here suggest that the author may have thought otherwise.

Removed the ‘and’ here.

> (Line 996 is similar.)

Wrapped it and the recursive call to lua-find-matching-token-word
on the next line in progn to make it clear the goto-char is for
side-effect and not return value.

> 2099         for type = (if (string-match-p "\\`(E" msg) :error :warning)
>
> A regexp is overkill here; `string-prefix-p` is simpler.

The plan is to merge the common parts between the lua modes. The
lua-ts-mode flymake backend uses string-prefix-p there so I left
this for now and it’ll get replaced with the lua-ts flymake code.

> 1821       (while (re-search-forward "[\"'\\\t\\\n]" nil t)
>
> This regexp is a bit muddled; too many backslashes here.
> You could even rewrite the whole function using `replace-regexp-in-string`, maybe
>
>   (concat "'"
>           (replace-regexp-in-string
>            (rx (or ?\" ?' ?\t ?\n ?\\))
>            (lambda (s)
>              (cdr (assq (aref s 0) '((?\" . "\\\"")
>                                      (?\\ . "\\\\")
>                                      (?\n . "\\n")
>                                      (?\t . "\\t")
>                                      (?'  . "\\'")))))
>            str t t)
>           "'"))
>
> without any need for a temporary buffer.

That works.

[0001-Various-code-cleanup-in-lua-mode-Bug-79309.patch (text/x-patch, attachment)]

Message #16 received at 79309-done <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: john muhl <jm <at> pub.pink>
Cc: 79309-done <at> debbugs.gnu.org
Subject: Re: bug#79309: lua-mode: minor problems and suggestions
Date: Wed, 27 Aug 2025 10:15:50 +0200

27 aug. 2025 kl. 05.35 skrev john muhl <jm <at> pub.pink>:

> Thanks for the review. Attached patch fixes these and a few other
> little things.

Looks good, thank you! Pushing to master and closing bug.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.