GNU bug report logs - #28803
[PATCH] Fixed compiler warnings for advised functions.

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 28803 in the body.
You can then email your comments to 28803 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: John Williams <jrw <at> pobox.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fixed compiler warnings for advised functions.
Date: Thu, 12 Oct 2017 16:02:39 -0700

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

[0001-Fixed-compiler-warnings-for-advised-functions.patch (text/x-patch, attachment)]

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

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: John Williams <jrw <at> pobox.com>
Cc: 28803 <at> debbugs.gnu.org
Subject: Re: bug#28803: [PATCH] Fixed compiler warnings for advised functions.
Date: Sat, 14 Oct 2017 01:51:18 -0400

John Williams <jrw <at> pobox.com> writes:

> Subject: [PATCH] Fixed compiler warnings for advised functions.

I think this bug was already fixed in emacs-26, see [1: 6e2d6d54e1],
Bug#14860 (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=14860).

[1: 6e2d6d54e1]: 2017-07-14 11:27:21 -0400
  * lisp/emacs-lisp/bytecomp.el: Fix bug#14860.
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=6e2d6d54e1236216462c13655ea1fe573d9672e7

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

From: John Williams <jrw <at> pobox.com>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 28803 <at> debbugs.gnu.org
Subject: Re: bug#28803: [PATCH] Fixed compiler warnings for advised functions.
Date: Sat, 14 Oct 2017 15:55:21 -0700

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

Oops. Is there anything that can be salvaged from my patch? Aside from
fixing the bug, it also adds a unit test and refactors the logic for
finding a function's argument list into a separate function that's not part
of the help system.

On Oct 13, 2017 10:51 PM, "Noam Postavsky" <npostavs <at> users.sourceforge.net>
wrote:

John Williams <jrw <at> pobox.com> writes:

> Subject: [PATCH] Fixed compiler warnings for advised functions.

I think this bug was already fixed in emacs-26, see [1: 6e2d6d54e1],
Bug#14860 (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=14860).

[1: 6e2d6d54e1]: 2017-07-14 11:27:21 -0400
  * lisp/emacs-lisp/bytecomp.el: Fix bug#14860.
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=
6e2d6d54e1236216462c13655ea1fe573d9672e7

[Message part 2 (text/html, inline)]

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

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: John Williams <jrw <at> pobox.com>
Cc: 28803 <at> debbugs.gnu.org
Subject: Re: bug#28803: [PATCH] Fixed compiler warnings for advised functions.
Date: Sat, 14 Oct 2017 19:47:15 -0400

John Williams <jrw <at> pobox.com> writes:

> Oops. Is there anything that can be salvaged from my patch? Aside
> from fixing the bug, it also adds a unit test and refactors the logic
> for finding a function's argument list into a separate function
> that's not part of the help system.

We could add the test, it seems to be passing in emacs-26.  Have you
assigned copyright to Emacs?  (It's okay if you haven't, the patch is
small enough to go in regardless, we would just need to mark it.)

I don't think there's much need for moving the function argument
retrieval out of the help system.

(By the way, if you've spent some time looking at help-function-arglist,
perhaps you have some ideas about Bug#26270?
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26270)

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

From: John Williams <jrw <at> pobox.com>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 28803 <at> debbugs.gnu.org
Subject: Re: bug#28803: [PATCH] Fixed compiler warnings for advised functions.
Date: Sat, 14 Oct 2017 17:30:46 -0700

On Sat, Oct 14, 2017 at 4:47 PM, Noam Postavsky
<npostavs <at> users.sourceforge.net> wrote:
> John Williams <jrw <at> pobox.com> writes:
>
>> Oops. Is there anything that can be salvaged from my patch? Aside
>> from fixing the bug, it also adds a unit test and refactors the logic
>> for finding a function's argument list into a separate function
>> that's not part of the help system.
>
> We could add the test, it seems to be passing in emacs-26.  Have you
> assigned copyright to Emacs?  (It's okay if you haven't, the patch is
> small enough to go in regardless, we would just need to mark it.)

No; how would I go about doing that? The organization of the dev site
is a bit confusing to me.

> I don't think there's much need for moving the function argument
> retrieval out of the help system.

It's not a huge deal to me, but it seems weird that something like
nadvice.el would depend on the help system (which it would in my patch
if I hadn't moved that function--I assume the fix that was already
committed does something similar).

> (By the way, if you've spent some time looking at help-function-arglist,
> perhaps you have some ideas about Bug#26270?
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26270)

I took a look at your patch, and without having tried running it, it
seems pretty reasonable to me. A unit test of some sort would be nice,
but since there don't seem to be any for help-function-arglist yet,
accepting the patch as-is would leave the unit test situation no worse
than it was before. I'm not a regular contributor, though, so my
opinion is worth what you paid for it.

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

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: John Williams <jrw <at> pobox.com>
Cc: 28803 <at> debbugs.gnu.org
Subject: Re: bug#28803: [PATCH] Fixed compiler warnings for advised functions.
Date: Sat, 14 Oct 2017 21:00:40 -0400

John Williams <jrw <at> pobox.com> writes:

>> We could add the test, it seems to be passing in emacs-26.  Have you
>> assigned copyright to Emacs?  (It's okay if you haven't, the patch is
>> small enough to go in regardless, we would just need to mark it.)
>
> No; how would I go about doing that? The organization of the dev site
> is a bit confusing to me.

You fill in this form:
http://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.program
and send it to fsf-records <at> gnu.org, then wait for further instructions
(in my experience it takes a little over a month to get a response).

>> I don't think there's much need for moving the function argument
>> retrieval out of the help system.
>
> It's not a huge deal to me, but it seems weird that something like
> nadvice.el would depend on the help system (which it would in my patch
> if I hadn't moved that function--I assume the fix that was already
> committed does something similar).

The other patch doesn't change nadvice.el.  There is another existing
call to help-function-arglist in advice--make-docstring, but since it is
used to create a docstring, to me it seems appropriate to depend on the
help system.

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

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: John Williams <jrw <at> pobox.com>
Cc: 28803 <at> debbugs.gnu.org
Subject: Re: bug#28803: [PATCH] Fixed compiler warnings for advised functions.
Date: Sat, 21 Oct 2017 19:33:23 -0400

tags 28803 fixed
close 28803 26.1
unarchive 14860
merge 28803 14860
quit

Noam Postavsky <npostavs <at> users.sourceforge.net> writes:

> John Williams <jrw <at> pobox.com> writes:
>
>> Oops. Is there anything that can be salvaged from my patch? Aside
>> from fixing the bug, it also adds a unit test and refactors the logic
>> for finding a function's argument list into a separate function
>> that's not part of the help system.
>
> We could add the test, it seems to be passing in emacs-26.

I've pushed the test.

[1: 237e96bc52]: 2017-10-21 19:20:46 -0400
  Test that advice doesn't trigger bytecomp warnings (Bug#28803)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=237e96bc5259e59ac5623a93a47f64abffab4e0b

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

From: John Williams <jrw <at> pobox.com>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 28803 <at> debbugs.gnu.org
Subject: Re: bug#28803: [PATCH] Fixed compiler warnings for advised functions.
Date: Sat, 21 Oct 2017 19:23:35 -0700

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

Hooray, my first official contribution to Emacs! Now I have code—not much,
but some—in both Emacs and Vim, which I have to think is pretty rare.

BTW, is posting to this list really the best way to send patches? At my
job, all changes go through a code review tool similar to Gerrit
<https://www.gerritcodereview.com/>, with code uploaded using a command
analogous to "git push", so sending a patch as an email attachment feels
clumsy and anachronistic.

On Oct 21, 2017 4:33 PM, "Noam Postavsky" <npostavs <at> users.sourceforge.net>
wrote:

> tags 28803 fixed
> close 28803 26.1
> unarchive 14860
> merge 28803 14860
> quit
>
> Noam Postavsky <npostavs <at> users.sourceforge.net> writes:
>
> > John Williams <jrw <at> pobox.com> writes:
> >
> >> Oops. Is there anything that can be salvaged from my patch? Aside
> >> from fixing the bug, it also adds a unit test and refactors the logic
> >> for finding a function's argument list into a separate function
> >> that's not part of the help system.
> >
> > We could add the test, it seems to be passing in emacs-26.
>
> I've pushed the test.
>
> [1: 237e96bc52]: 2017-10-21 19:20:46 -0400
>   Test that advice doesn't trigger bytecomp warnings (Bug#28803)
>   https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=237e9
> 6bc5259e59ac5623a93a47f64abffab4e0b
>

[Message part 2 (text/html, inline)]

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

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: John Williams <jrw <at> pobox.com>
Cc: 28803 <at> debbugs.gnu.org
Subject: Re: bug#28803: [PATCH] Fixed compiler warnings for advised functions.
Date: Sun, 22 Oct 2017 09:04:01 -0400

John Williams <jrw <at> pobox.com> writes:

> Hooray, my first official contribution to Emacs! Now I have code—not
> much, but some—in both Emacs and Vim, which I have to think is pretty
> rare.

Congrats :)

> BTW, is posting to this list really the best way to send patches? At
> my job, all changes go through a code review tool similar to Gerrit,
> with code uploaded using a command analogous to "git push", so
> sending a patch as an email attachment feels clumsy and
> anachronistic.

For now at least yes.  There has been some grumbling from others about
the "anachronistic" methods, but nothing much has come of it as of yet.

I have some commands in my init file for smoothing the process of
sending and receiving patches between the bug list and my git repo, but
they're dependent of magit, and I'm still working out the kinks.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: John Williams <jrw <at> pobox.com>
Cc: 28803 <at> debbugs.gnu.org, npostavs <at> users.sourceforge.net
Subject: Re: bug#28803: [PATCH] Fixed compiler warnings for advised functions.
Date: Sun, 22 Oct 2017 17:04:47 +0300

> From: John Williams <jrw <at> pobox.com>
> Date: Sat, 21 Oct 2017 19:23:35 -0700
> Cc: 28803 <at> debbugs.gnu.org
> 
> Hooray, my first official contribution to Emacs! Now I have code—not much, but some—in both Emacs and
> Vim, which I have to think is pretty rare.

Welcome on board, and thanks for your contribution.

> BTW, is posting to this list really the best way to send patches?

You are not posting to a list, you are sending to our issue tracker.
Having your patches sent to a mailing list is just one of the
interfaces provided by the tracker; there are others (like an Emacs
interface, see the debbugs package in ELPA.

> At my job, all changes go through a code
> review tool similar to Gerrit, with code uploaded using a command analogous to "git push", so sending a patch
> as an email attachment feels clumsy and anachronistic.

For large changesets, we recommend a scratch branch.

Previous Next

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