GNU bug report logs - #46889
cperl-mode: Fix indentation issues [PATCH]

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 46889 in the body.
You can then email your comments to 46889 AT debbugs.gnu.org in the normal way.

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

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

From: haj <at> posteo.de (Harald Jörg)
To: bug-gnu-emacs <at> gnu.org
Subject: cperl-mode: Fix indentation issues [PATCH]
Date: Wed, 03 Mar 2021 15:43:10 +0100

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

Hello Emacs-Maintainers,

this started out as working on Bug#42169 but it soon became clear that
there's more lurking in CPerl mode's indentation code, so I decided to
open a new issue here.

In 7b2448ae (cperl-mode: Improve detection of index entries for imenu),
committed on Feb 17, the imenu part of Bug#42169 has been fixed.  This
commit now fixes the indentation after a package which brings its own
block.

Therefore, the patch in this report should finally fix Bug#42169.

As a by-catch, the new code fixes the ancient Bug#8077.

This patch also fixes the first part of Bug#11733.  The "other
indentation bug" reported in Bug#11733 (using a colon as a Perl regexp
terminator) is a different story and not covered by this patch.

Also fixed now (without known bug number): labels with a space before,
or a comment after the colon are now correctly indented and fontified.

With this patch in effect, CPerl mode should finally have caught up with
Perl 5.30!

Many thanks to Mattias Engdegård for his patient explanations how rx
expressions can be used without breaking compatibility to Emacs 26.1.
Until now, this allowed two ugly regexp literals to be eliminated.
-- 
Cheers,
haj

[0001-cperl-mode-fix-indentation-issues.patch (text/x-diff, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: haj <at> posteo.de (Harald Jörg)
Cc: 46889 <at> debbugs.gnu.org,
 Mattias Engdegård <mattiase <at> acm.org>
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Thu, 04 Mar 2021 18:31:37 +0100

haj <at> posteo.de (Harald Jörg) writes:

> As a by-catch, the new code fixes the ancient Bug#8077.
>
> This patch also fixes the first part of Bug#11733.  The "other
> indentation bug" reported in Bug#11733 (using a colon as a Perl regexp
> terminator) is a different story and not covered by this patch.
>
> Also fixed now (without known bug number): labels with a space before,
> or a comment after the colon are now correctly indented and fontified.

Sounds good.

> With this patch in effect, CPerl mode should finally have caught up with
> Perl 5.30!

:-)

> Many thanks to Mattias Engdegård for his patient explanations how rx
> expressions can be used without breaking compatibility to Emacs 26.1.
> Until now, this allowed two ugly regexp literals to be eliminated.

Funnily enough, that's the one thing I was going to ask you about with
this patch.

I'm not an experienced rx user -- at all -- but is this the best way to
do this?

> +(eval-and-compile
> +  (defconst cperl--ws-rx
> +    '(sequence (or space "\n"))

It was the eval-and-compile that made me look at this bit...

[...]

> +(defconst cperl-maybe-white-and-comment-rex
> +  (rx-to-string `(group (eval cperl--ws*-rx)))

I'm thinking of the rx `eval' things everywhere.  Wouldn't defining
these rx expressions with `rx-define' be more straighforward, and then
you'd have:

> +(defconst cperl-maybe-white-and-comment-rex
> +  (rx-to-string `(group cperl--ws*-rx))

(Similarly for basically all the rx `eval' bits.

Now, rx expands the `eval' at compile time, I think, so it doesn't
really make that much difference in practice, but it seems slightly
clearer, perhaps?

(Added Mattias to the CCs.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Mattias Engdegård <mattiase <at> acm.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 46889 <at> debbugs.gnu.org, Harald Jörg <haj <at> posteo.de>
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Thu, 4 Mar 2021 19:07:28 +0100

4 mars 2021 kl. 18.31 skrev Lars Ingebrigtsen <larsi <at> gnus.org>:

> I'm thinking of the rx `eval' things everywhere.  Wouldn't defining
> these rx expressions with `rx-define' be more straighforward, and then
> you'd have:

`rx-define` is new in Emacs 27, and the patch was apparently intended to work with 26.1.

>> +(defconst cperl-maybe-white-and-comment-rex
>> +  (rx-to-string `(group cperl--ws*-rx))
> 
> (Similarly for basically all the rx `eval' bits.
> 
> Now, rx expands the `eval' at compile time, I think, so it doesn't
> really make that much difference in practice, but it seems slightly
> clearer, perhaps?

Even in Emacs 26, there are several more or less equivalent ways of expressing the same thing. I'm sure it will be fine.

A minor detail -- ignore it if you like -- is that some backquote forms now no longer have any substitutions (, or ,@) and could therefore be plain quoted forms. Or, if you prefer, you could keep the backquotes and use comma-substitution instead of `eval` instead of mixing the two. It may also be a good idea to use `rx` instead of `rx-to-string` in some places to ensure that the translation is done at compile time instead of when the module is loaded.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 46889 <at> debbugs.gnu.org, Harald Jörg <haj <at> posteo.de>
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Thu, 04 Mar 2021 19:16:29 +0100

Mattias Engdegård <mattiase <at> acm.org> writes:

> `rx-define` is new in Emacs 27, and the patch was apparently intended
> to work with 26.1.

I was fooled by the version guesser:

---
rx-define is an autoloaded Lisp macro in ‘rx.el’.

(rx-define NAME [(ARGS...)] RX)

  Probably introduced at or before Emacs version 24.
---

I wonder where that is coming from...  rx-define isn't mentioned in
NEWS.24?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: haj <at> posteo.de (Harald Jörg)
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 46889 <at> debbugs.gnu.org,
 Mattias Engdegård <mattiase <at> acm.org>
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Thu, 04 Mar 2021 19:19:42 +0100

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> [...]
>> Many thanks to Mattias Engdegård for his patient explanations how rx
>> expressions can be used without breaking compatibility to Emacs 26.1.
>> Until now, this allowed two ugly regexp literals to be eliminated.
>
> Funnily enough, that's the one thing I was going to ask you about with
> this patch.
>
> I'm not an experienced rx user -- at all -- but is this the best way to
> do this?
>
>> +(eval-and-compile
>> +  (defconst cperl--ws-rx
>> +    '(sequence (or space "\n"))
>
> It was the eval-and-compile that made me look at this bit...

I am not an experienced rx user at all, I only recently discovered it :)
I'm always happy when somebody else is looking over my changes.

The patch was a result of a my understanding of an off-the-lists dialog
between Mattias and me, where it is totally possible that I
misunderstood or misimplemented things.

What led me to eval-and-compile was that I want to "compose" Perl syntax
elements from those defined earlier - and I also want to write unit
tests for the rx form in cperl-mode-tests.el.

> [...]
>
>> +(defconst cperl-maybe-white-and-comment-rex
>> +  (rx-to-string `(group (eval cperl--ws*-rx)))
>
> I'm thinking of the rx `eval' things everywhere.  Wouldn't defining
> these rx expressions with `rx-define' be more straighforward, and then
> you'd have:

Yes, it would be.  The downside is that I'd have to abandon the plan to
move CPerl mode to ELPA and make it available for Emacs 26.1 and up.
Perl coders might want to try the new CPerl mode without having to build
Emacs 28 - and I want their feedback.

I'm ready to upgrade that stuff to `rx-define' when the typical Linux
distributions ship with an Emacs version supporting it!

>> +(defconst cperl-maybe-white-and-comment-rex
>> +  (rx-to-string `(group cperl--ws*-rx))
>
> (Similarly for basically all the rx `eval' bits.
>
> Now, rx expands the `eval' at compile time, I think, so it doesn't
> really make that much difference in practice, but it seems slightly
> clearer, perhaps?

Sure, I understand that.

By the way, the journey isn't over yet.  What I plan to do next is to
add support for popular Perl extensions which bring new syntax (also, a
new Perl version might sooner or later bring the same keywords, probably
with "better" syntax).  So I guess there will be a basic
compile-time-syntax, plus modifications at runtime when I know which
exact syntax applies to a buffer.
-- 
Cheers,
haj

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

From: Mattias Engdegård <mattiase <at> acm.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 46889 <at> debbugs.gnu.org, Harald Jörg <haj <at> posteo.de>
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Thu, 4 Mar 2021 19:23:43 +0100

4 mars 2021 kl. 19.16 skrev Lars Ingebrigtsen <larsi <at> gnus.org>:

> I wonder where that is coming from...  rx-define isn't mentioned in
> NEWS.24?

I don't know. Is there a way to override the guesser and provide an explicit version?

Similarly, if an argument has been added or extended, wouldn't it be useful to add this to the documentation in some way?

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 46889 <at> debbugs.gnu.org, Harald Jörg <haj <at> posteo.de>
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Thu, 04 Mar 2021 19:34:27 +0100

Mattias Engdegård <mattiase <at> acm.org> writes:

> I don't know. Is there a way to override the guesser and provide an
> explicit version?
>
> Similarly, if an argument has been added or extended, wouldn't it be
> useful to add this to the documentation in some way?

Both are more work than we're willing to do.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 46889 <at> debbugs.gnu.org,
 Mattias Engdegård <mattiase <at> acm.org>,
 Harald Jörg <haj <at> posteo.de>
Subject: Help version guesser bug (was: bug#46889: cperl-mode: Fix
 indentation issues [PATCH])
Date: Fri, 05 Mar 2021 11:30:06 +0100

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

On Thu, 04 Mar 2021 19:16:29 +0100 Lars Ingebrigtsen <larsi <at> gnus.org> wrote:

> Mattias Engdegård <mattiase <at> acm.org> writes:
>
>> `rx-define` is new in Emacs 27, and the patch was apparently intended
>> to work with 26.1.
>
> I was fooled by the version guesser:
>
> ---
> rx-define is an autoloaded Lisp macro in ‘rx.el’.
>
> (rx-define NAME [(ARGS...)] RX)
>
>   Probably introduced at or before Emacs version 24.
> ---
>
> I wonder where that is coming from...  rx-define isn't mentioned in
> NEWS.24?

It's because help-fns--first-release finds this line in NEWS.27:

** Further functions and variables obsolete since Emacs 24 have been removed:

On the assumption that in the NEWS files a first-level outline header
contains the relevant reference to the first release and doesn't contain
a reference to an earlier release, the following patch fixes the bug:

[Message part 2 (text/x-patch, inline)]

diff --git a/lisp/help-fns.el b/lisp/help-fns.el
index 290bebf7e5..01d3756bf0 100644
--- a/lisp/help-fns.el
+++ b/lisp/help-fns.el
@@ -668,7 +668,7 @@ help-fns--first-release
               ;; Almost all entries are of the form "* ... in Emacs NN.MM."
               ;; but there are also a few in the form "* Emacs NN.MM is a bug
               ;; fix release ...".
-              (if (not (re-search-backward "^\\*.* Emacs \\([0-9.]+[0-9]\\)"
+              (if (not (re-search-backward "^\\* .* Emacs \\([0-9.]+[0-9]\\)"
                                            nil t))
                   (message "Ref found in non-versioned section in %S"
                            (file-name-nondirectory f))

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

Steve Berman

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 46889 <at> debbugs.gnu.org,
 Mattias Engdegård <mattiase <at> acm.org>,
 Harald Jörg <haj <at> posteo.de>
Subject: Re: Help version guesser bug
Date: Fri, 05 Mar 2021 14:06:33 +0100

Stephen Berman <stephen.berman <at> gmx.net> writes:

> It's because help-fns--first-release finds this line in NEWS.27:
>
> ** Further functions and variables obsolete since Emacs 24 have been removed:

Ah, I see.

> On the assumption that in the NEWS files a first-level outline header
> contains the relevant reference to the first release and doesn't contain
> a reference to an earlier release, the following patch fixes the bug:

I've grepped through the NEWS files, and it looks like this assumption
is correct, so I've applied your patch.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: haj <at> posteo.de (Harald Jörg)
To: 46889 <at> debbugs.gnu.org
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Tue, 09 Mar 2021 18:06:34 +0100

haj <at> posteo.de (Harald Jörg) writes:

> Hello Emacs-Maintainers,
>
> this started out as working on Bug#42169 but it soon became clear that
> there's more lurking in CPerl mode's indentation code, so I decided to
> open a new issue here.
>
> In 7b2448ae (cperl-mode: Improve detection of index entries for imenu),
> committed on Feb 17, the imenu part of Bug#42169 has been fixed.  This
> commit now fixes the indentation after a package which brings its own
> block.
>
> Therefore, the patch in this report should finally fix Bug#42169.
>
> As a by-catch, the new code fixes the ancient Bug#8077.
>
> This patch also fixes the first part of Bug#11733.  The "other
> indentation bug" reported in Bug#11733 (using a colon as a Perl regexp
> terminator) is a different story and not covered by this patch.
>
> Also fixed now (without known bug number): labels with a space before,
> or a comment after the colon are now correctly indented and fontified.
>
> With this patch in effect, CPerl mode should finally have caught up with
> Perl 5.30!
>
> Many thanks to Mattias Engdegård for his patient explanations how rx
> expressions can be used without breaking compatibility to Emacs 26.1.
> Until now, this allowed two ugly regexp literals to be eliminated.

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

From: haj <at> posteo.de (Harald Jörg)
To: 46889 <at> debbugs.gnu.org
Cc: Mattias Engdegård <mattiase <at> acm.org>,
 Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Tue, 09 Mar 2021 18:18:47 +0100

Sorry, I accidently re-sent my initial report instead of this reply.

My patch has not been merged yet, and this is just as well: I'd like to
withdraw it and re-work parts of it.  This will take a day or two (or
more).  I also found that I've introduced a (minor) fontification bug.
-- 
Cheers, and sorry for the noise,
haj

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: haj <at> posteo.de (Harald Jörg)
Cc: 46889 <at> debbugs.gnu.org,
 Mattias Engdegård <mattiase <at> acm.org>
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Wed, 10 Mar 2021 15:42:31 +0100

haj <at> posteo.de (Harald Jörg) writes:

> My patch has not been merged yet, and this is just as well: I'd like to
> withdraw it and re-work parts of it.  This will take a day or two (or
> more).  I also found that I've introduced a (minor) fontification bug.

Sure; no problem.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: haj <at> posteo.de (Harald Jörg)
Cc: 46889 <at> debbugs.gnu.org,
 Mattias Engdegård <mattiase <at> acm.org>
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Mon, 17 May 2021 17:28:53 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> haj <at> posteo.de (Harald Jörg) writes:
>
>> My patch has not been merged yet, and this is just as well: I'd like to
>> withdraw it and re-work parts of it.  This will take a day or two (or
>> more).  I also found that I've introduced a (minor) fontification bug.
>
> Sure; no problem.

This was two months ago.  Did you make any progress here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: haj <at> posteo.de (Harald Jörg)
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 46889 <at> debbugs.gnu.org,
 Mattias Engdegård <mattiase <at> acm.org>
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Mon, 17 May 2021 21:31:34 +0000

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

>> haj <at> posteo.de (Harald Jörg) writes:
>>
>>> My patch has not been merged yet, and this is just as well: I'd like to
>>> withdraw it and re-work parts of it.  This will take a day or two (or
>>> more).  I also found that I've introduced a (minor) fontification bug.
>>
>> Sure; no problem.
>
> This was two months ago.  Did you make any progress here?

Oh - thanks for the reminder.  I forgot that this bug has the "patch"
tag set.

I did make progress - but not enough (yet) and also in a slightly
different direction.

That patch rolled three different stories into one:
 1) replace regex strings by rx forms as suggested by Mattias
 2) fix some indentation bugs (but broke some fontification regexes)
 3) start a mechanism to allow cperl-mode to understand syntax
    extensions.

This wasn't a good idea.

I'd like to split that into at least three separate commits (which I can
do by myself now, since I'm a member of the Emacs group now.  Thanks!).

I suggest to throw this patch away.  There's no specific bug in this bug
report number 46889.  I used the bug mechanism to provide a patch
without commit rights, but that path is no longer needed.  When a commit
fixes one of the old open bugs, I can just report that in the commit
message.

Thanks for your patience!
-- 
Cheers,
haj

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: haj <at> posteo.de (Harald Jörg)
Cc: 46889 <at> debbugs.gnu.org,
 Mattias Engdegård <mattiase <at> acm.org>
Subject: Re: bug#46889: cperl-mode: Fix indentation issues [PATCH]
Date: Tue, 18 May 2021 15:47:20 +0200

haj <at> posteo.de (Harald Jörg) writes:

> Oh - thanks for the reminder.  I forgot that this bug has the "patch"
> tag set.

Yes, I've been slowly going through all the open bugs marked as having a
patch.  :-)

> I suggest to throw this patch away.  There's no specific bug in this bug
> report number 46889.  I used the bug mechanism to provide a patch
> without commit rights, but that path is no longer needed.  When a commit
> fixes one of the old open bugs, I can just report that in the commit
> message.
>
> Thanks for your patience!

No problem.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

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