Re: master b2416d2c029 4/6: Don't load comp when installing an existing trampoline

Re: master b2416d2c029 4/6: Don't load comp when installing an existing trampoline

[Stefan Monnier]

Hi Andrea,

> --- a/lisp/emacs-lisp/nadvice.el
> +++ b/lisp/emacs-lisp/nadvice.el
> @@ -389,7 +389,7 @@ is also interactive.  There are 3 cases:
>    `(advice--add-function ,how (gv-ref ,(advice--normalize-place place))
>                           ,function ,props))
>  
> -(declare-function comp-subr-trampoline-install "comp")
> +(declare-function comp-subr-trampoline-install "comp-run")
>  
>  ;;;###autoload
>  (defun advice--add-function (how ref function props)
> @@ -407,7 +407,7 @@ is also interactive.  There are 3 cases:
>        (unless (memq subr-name '(macroexpand rename-buffer))
>          ;; Must require explicitly as during bootstrap we have no
>          ;; autoloads.
> -        (require 'comp)
> +        (require 'comp-run)
>          (comp-subr-trampoline-install subr-name))))

2 things:

- The `declare-function` should be moved to right after (require
  'comp-run), i.e. when we do know that the function should be available
  and it will thus silence only spurious warnings.
- Why do we do that in `advice--add-function`, which is used by
  `advice-add` indeed but also by `add-function`, e.g. when adding
  a function to a `foo-function`variable or to a process filter,
  where there's no trampoline to install.
- Why do we need this at all?
  Won't `Ffset` call `comp-subr-trampoline-install` for us anyway.


        Stefan

Re: master b2416d2c029 4/6: Don't load comp when installing an existing trampoline

[Andrea Corallo]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Hi Andrea,

Hi!

>
>> --- a/lisp/emacs-lisp/nadvice.el
>> +++ b/lisp/emacs-lisp/nadvice.el
>> @@ -389,7 +389,7 @@ is also interactive.  There are 3 cases:
>>    `(advice--add-function ,how (gv-ref ,(advice--normalize-place place))
>>                           ,function ,props))
>>  
>> -(declare-function comp-subr-trampoline-install "comp")
>> +(declare-function comp-subr-trampoline-install "comp-run")
>>  
>>  ;;;###autoload
>>  (defun advice--add-function (how ref function props)
>> @@ -407,7 +407,7 @@ is also interactive.  There are 3 cases:
>>        (unless (memq subr-name '(macroexpand rename-buffer))
>>          ;; Must require explicitly as during bootstrap we have no
>>          ;; autoloads.
>> -        (require 'comp)
>> +        (require 'comp-run)
>>          (comp-subr-trampoline-install subr-name))))
>
> 2 things:
>
> - The `declare-function` should be moved to right after (require
>   'comp-run), i.e. when we do know that the function should be available
>   and it will thus silence only spurious warnings.

Ack will do, out of curiosity what is the downside of having the
declare-function at top level?  I thought is there to silence a compile
time warning (and thus the branch is inserted in should not play a
role).

> - Why do we do that in `advice--add-function`, which is used by
>   `advice-add` indeed but also by `add-function`, e.g. when adding
>   a function to a `foo-function`variable or to a process filter,
>   where there's no trampoline to install.
> - Why do we need this at all?
>   Won't `Ffset` call `comp-subr-trampoline-install` for us anyway.

As mentioned in the other thread I fear I don't remeber the answer to
those quesitons.  The best we can do is to remove the call and test
Emacs to find if there's still a good reason.

Bests!

  Andrea

Re: master b2416d2c029 4/6: Don't load comp when installing an existing trampoline

[Stefan Monnier]

>> - The `declare-function` should be moved to right after (require
>>   'comp-run), i.e. when we do know that the function should be available
>>   and it will thus silence only spurious warnings.
>
> Ack will do, out of curiosity what is the downside of having the
> declare-function at top level?  I thought is there to silence a compile
> time warning (and thus the branch is inserted in should not play a
> role).

If you call that function from elsewhere in the file, there will be no
compilation warning, even though the call may error at run time because
`comp-run` wasn't loaded.

By moving the `declare-function` you make sure only those warnings that
you know for sure are spurious (because we just did `(require
'comp-run)`) will get silenced.

> As mentioned in the other thread I fear I don't remeber the answer to
> those quesitons.  The best we can do is to remove the call and test
> Emacs to find if there's still a good reason.

Then let's remove it in `master`.  This way, if it's needed we'll get to
see why/when and we'll be able to at the very least add a good comment,
or maybe replace it with a better solution.


        Stefan

Re: master b2416d2c029 4/6: Don't load comp when installing an existing trampoline

[Andrea Corallo]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> - The `declare-function` should be moved to right after (require
>>>   'comp-run), i.e. when we do know that the function should be available
>>>   and it will thus silence only spurious warnings.
>>
>> Ack will do, out of curiosity what is the downside of having the
>> declare-function at top level?  I thought is there to silence a compile
>> time warning (and thus the branch is inserted in should not play a
>> role).
>
> If you call that function from elsewhere in the file, there will be no
> compilation warning, even though the call may error at run time because
> `comp-run` wasn't loaded.
>
> By moving the `declare-function` you make sure only those warnings that
> you know for sure are spurious (because we just did `(require
> 'comp-run)`) will get silenced.

Thanks for the explaination, done.

>> As mentioned in the other thread I fear I don't remeber the answer to
>> those quesitons.  The best we can do is to remove the call and test
>> Emacs to find if there's still a good reason.
>
> Then let's remove it in `master`.  This way, if it's needed we'll get to
> see why/when and we'll be able to at the very least add a good comment,
> or maybe replace it with a better solution.

Ack, I'll try it before in my machine and push it if I see no
regressions here.

Thanks

  Andrea

Re: master b2416d2c029 4/6: Don't load comp when installing an existing trampoline

[Andrea Corallo]

Andrea Corallo <acorallo@gnu.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>> - The `declare-function` should be moved to right after (require
>>>>   'comp-run), i.e. when we do know that the function should be available
>>>>   and it will thus silence only spurious warnings.
>>>
>>> Ack will do, out of curiosity what is the downside of having the
>>> declare-function at top level?  I thought is there to silence a compile
>>> time warning (and thus the branch is inserted in should not play a
>>> role).
>>
>> If you call that function from elsewhere in the file, there will be no
>> compilation warning, even though the call may error at run time because
>> `comp-run` wasn't loaded.
>>
>> By moving the `declare-function` you make sure only those warnings that
>> you know for sure are spurious (because we just did `(require
>> 'comp-run)`) will get silenced.
>
> Thanks for the explaination, done.
>
>>> As mentioned in the other thread I fear I don't remeber the answer to
>>> those quesitons.  The best we can do is to remove the call and test
>>> Emacs to find if there's still a good reason.
>>
>> Then let's remove it in `master`.  This way, if it's needed we'll get to
>> see why/when and we'll be able to at the very least add a good comment,
>> or maybe replace it with a better solution.
>
> Ack, I'll try it before in my machine and push it if I see no
> regressions here.

Okay doing some git archeology I discovered that at the time during
development when I added the code into 'advice--add-function' we had no
handling in Fset.  This makes me more confident at thinking that the
special handling is not necessary.  Tests are passing so I pushed the
clean-up.

Thanks!

  Andrea