* Re: master b2416d2c029 4/6: Don't load comp when installing an existing trampoline [not found] ` <20231109113503.9890EC0C198@vcs2.savannah.gnu.org> @ 2023-11-13 18:46 ` Stefan Monnier 2023-11-14 9:33 ` Andrea Corallo 0 siblings, 1 reply; 5+ messages in thread From: Stefan Monnier @ 2023-11-13 18:46 UTC (permalink / raw) To: Andrea Corallo; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: master b2416d2c029 4/6: Don't load comp when installing an existing trampoline 2023-11-13 18:46 ` master b2416d2c029 4/6: Don't load comp when installing an existing trampoline Stefan Monnier @ 2023-11-14 9:33 ` Andrea Corallo 2023-11-14 14:07 ` Stefan Monnier 0 siblings, 1 reply; 5+ messages in thread From: Andrea Corallo @ 2023-11-14 9:33 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: master b2416d2c029 4/6: Don't load comp when installing an existing trampoline 2023-11-14 9:33 ` Andrea Corallo @ 2023-11-14 14:07 ` Stefan Monnier 2023-11-16 8:35 ` Andrea Corallo 0 siblings, 1 reply; 5+ messages in thread From: Stefan Monnier @ 2023-11-14 14:07 UTC (permalink / raw) To: Andrea Corallo; +Cc: emacs-devel >> - 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: master b2416d2c029 4/6: Don't load comp when installing an existing trampoline 2023-11-14 14:07 ` Stefan Monnier @ 2023-11-16 8:35 ` Andrea Corallo 2023-11-16 9:34 ` Andrea Corallo 0 siblings, 1 reply; 5+ messages in thread From: Andrea Corallo @ 2023-11-16 8:35 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: master b2416d2c029 4/6: Don't load comp when installing an existing trampoline 2023-11-16 8:35 ` Andrea Corallo @ 2023-11-16 9:34 ` Andrea Corallo 0 siblings, 0 replies; 5+ messages in thread From: Andrea Corallo @ 2023-11-16 9:34 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-11-16 9:34 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <169952969842.2327.17551267288789292121@vcs2.savannah.gnu.org> [not found] ` <20231109113503.9890EC0C198@vcs2.savannah.gnu.org> 2023-11-13 18:46 ` master b2416d2c029 4/6: Don't load comp when installing an existing trampoline Stefan Monnier 2023-11-14 9:33 ` Andrea Corallo 2023-11-14 14:07 ` Stefan Monnier 2023-11-16 8:35 ` Andrea Corallo 2023-11-16 9:34 ` Andrea Corallo
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.