Suspicious code in align.el

Suspicious code in align.el

[Artur Malabarba]

In line 890 of this file, there's a call to align-regexp that looks like this:

(align-region beg end
              (or exclude-rules
                  align-mode-exclude-rules-list
                  align-exclude-rules-list)
              nil
              separator
              (function
               (lambda (b e mode)
                 (when (and mode (listp mode))
                   (setq sec-first (min sec-first b)
                         sec-last  (max sec-last e))))))

Note how the separator here is passed as 5th argument, while the
docstring of `align-region' documents it as the 3th arg. Furthermore,
the exclude-rules are passed as 3rd arg, but they're documented as
5th.
Is this correct?

Apparently this was written 15 years ago, so this means I'm either
missing something or this code branch is never reached.

Cheers,
Artur

Re: Suspicious code in align.el

[John Wiegley]

>>>>> Artur Malabarba <bruce.connor.am@gmail.com> writes:

> In line 890 of this file, there's a call to align-regexp that looks like
> this...

Nice catch, Artur, the code at least is wrong. The 3rd argument should be
shifted forward to become the 5th argument.

I wonder what fixing this will break... :)

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Suspicious code in align.el

[Artur Malabarba]

2016-01-02 22:18 GMT+00:00 John Wiegley <jwiegley@gmail.com>:
>>>>>> Artur Malabarba <bruce.connor.am@gmail.com> writes:
>
>> In line 890 of this file, there's a call to align-regexp that looks like
>> this...
>
> Nice catch, Artur, the code at least is wrong. The 3rd argument should be
> shifted forward to become the 5th argument.
>
> I wonder what fixing this will break... :)

Since it's possible this will break something else, should this "fix"
go on master instead of the release branch?

Re: Suspicious code in align.el

[Eli Zaretskii]

> Date: Sun, 3 Jan 2016 15:54:57 +0000
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> 
> 2016-01-02 22:18 GMT+00:00 John Wiegley <jwiegley@gmail.com>:
> >>>>>> Artur Malabarba <bruce.connor.am@gmail.com> writes:
> >
> >> In line 890 of this file, there's a call to align-regexp that looks like
> >> this...
> >
> > Nice catch, Artur, the code at least is wrong. The 3rd argument should be
> > shifted forward to become the 5th argument.
> >
> > I wonder what fixing this will break... :)
> 
> Since it's possible this will break something else, should this "fix"
> go on master instead of the release branch?

No, emacs-25 is fine in this case.

Thanks.

Re: Suspicious code in align.el

[Artur Malabarba]

2016-01-03 16:40 GMT+00:00 Eli Zaretskii <eliz@gnu.org>:
> No, emacs-25 is fine in this case.
>
> Thanks.

pushed

Re: Suspicious code in align.el

[John Wiegley]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:

>> Since it's possible this will break something else, should this "fix"
>> go on master instead of the release branch?

> No, emacs-25 is fine in this case.

emacs-25 is where the fix should go.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2