bug#28803: [PATCH] Fixed compiler warnings for advised functions.

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#28803: [PATCH] Fixed compiler warnings for advised functions.

jrw@pobox.com


0001-Fixed-compiler-warnings-for-advised-functions.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#28803: [PATCH] Fixed compiler warnings for advised functions.

Noam Postavsky-2
John Williams <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

bug#28803: [PATCH] Fixed compiler warnings for advised functions.

jrw@pobox.com
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" <[hidden email]> wrote:
John Williams <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

bug#28803: [PATCH] Fixed compiler warnings for advised functions.

Noam Postavsky-2
John Williams <[hidden email]> 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)




Reply | Threaded
Open this post in threaded view
|

bug#28803: [PATCH] Fixed compiler warnings for advised functions.

jrw@pobox.com
On Sat, Oct 14, 2017 at 4:47 PM, Noam Postavsky
<[hidden email]> wrote:

> John Williams <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

bug#28803: [PATCH] Fixed compiler warnings for advised functions.

Noam Postavsky-2
John Williams <[hidden email]> 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 [hidden email], 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.