GNU bug report logs - #60311
json-available-p: make dynamically correct for Windows

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 60311 in the body.
You can then email your comments to 60311 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: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Emacs Bug Report <bug-gnu-emacs <at> gnu.org>
Subject: json-available-p: make dynamically correct for Windows
Date: Sun, 25 Dec 2022 15:32:06 +0100

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

The implementation of `json-available-p`,

  (and (fboundp 'json-serialize)
       (condition-case nil
           (json-serialize t)
         (:success t)
         (json-unavailable nil))))

probably isn't quite right on Windows: `json-serialize` is pure so it will be called at compile time and the result, "true", used in the code (actually not even that since the result is never used). Thus, if libjansson could not be loaded during actual Emacs use (as opposed to when Emacs was built), this would never be detected and json-available-p would still return t.

The patch below should take care of this. Would someone please help testing it on Windows?

[json-available-p.diff (application/octet-stream, attachment)]

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

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Emacs Bug Report <bug-gnu-emacs <at> gnu.org>
Subject: Re: json-available-p: make dynamically correct for Windows
Date: Sun, 25 Dec 2022 16:34:30 +0100

By the way, I'm entirely to blame for this flaw: a compiler bug only recently fixed (8bb8cc5b49) prevented the emission of a warning that would have alerted the author of `json-available-p` to the issue at hand.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattias.engdegard <at> gmail.com>
Cc: 60311 <at> debbugs.gnu.org
Subject: Re: bug#60311: json-available-p: make dynamically correct for Windows
Date: Sun, 25 Dec 2022 17:40:47 +0200

> From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
> Date: Sun, 25 Dec 2022 15:32:06 +0100
> 
> The implementation of `json-available-p`,
> 
>   (and (fboundp 'json-serialize)
>        (condition-case nil
>            (json-serialize t)
>          (:success t)
>          (json-unavailable nil))))
> 
> probably isn't quite right on Windows: `json-serialize` is pure so it will be called at compile time and the result, "true", used in the code (actually not even that since the result is never used). Thus, if libjansson could not be loaded during actual Emacs use (as opposed to when Emacs was built), this would never be detected and json-available-p would still return t.

Yes, you are right.  But please come up with a smaller changeset which
only changes what strictly needs to be changed.  Or if you want, I can
do this myself.

AFAIU, we just need a C implementation of json-available-p, since
doing this in Lisp doesn't work.  So that's the change I expect,
nothing more, nothing less.  Like we do with other optional libraries
and features, there's more than enough examples in the codebase.

Thanks.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: mattias.engdegard <at> gmail.com
Cc: 60311 <at> debbugs.gnu.org
Subject: Re: bug#60311: json-available-p: make dynamically correct for Windows
Date: Sun, 25 Dec 2022 18:24:54 +0200

> Cc: 60311 <at> debbugs.gnu.org
> Date: Sun, 25 Dec 2022 17:40:47 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> AFAIU, we just need a C implementation of json-available-p, since
> doing this in Lisp doesn't work.

This came out too ambiguous.  So here's a more accurate request:
please make your json_available_p use the same code as the first part
of the below:

  #ifdef WINDOWSNT
    if (!json_initialized)
      {
	Lisp_Object status;
	json_initialized = init_json_functions ();
	status = json_initialized ? Qt : Qnil;
	Vlibrary_cache = Fcons (Fcons (Qjson, status), Vlibrary_cache);
      }
    if (!json_initialized)
      Fsignal (Qjson_unavailable,
	       list1 (build_unibyte_string ("jansson library not found")));
  #endif

and your ensure_json_available use the second part of the above.
There's no reason to reshuffle the code that works.

Also, please make ensure_json_available specific to WINDOWSNT, so that
we don't pay an extra function call on Posix platforms.

And finally, json--available-p should be defined on all platforms that
compile json.c, not just on WINDOWSNT; it should return t on Posix
platforms.

Thanks.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: mattias.engdegard <at> gmail.com
Cc: 60311 <at> debbugs.gnu.org
Subject: Re: bug#60311: json-available-p: make dynamically correct for Windows
Date: Sun, 25 Dec 2022 19:26:59 +0200

> Cc: 60311 <at> debbugs.gnu.org
> Date: Sun, 25 Dec 2022 18:24:54 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> This came out too ambiguous.  So here's a more accurate request:
> please make your json_available_p use the same code as the first part
> of the below:
> 
>   #ifdef WINDOWSNT
>     if (!json_initialized)
>       {
> 	Lisp_Object status;
> 	json_initialized = init_json_functions ();
> 	status = json_initialized ? Qt : Qnil;
> 	Vlibrary_cache = Fcons (Fcons (Qjson, status), Vlibrary_cache);
>       }
>     if (!json_initialized)
>       Fsignal (Qjson_unavailable,
> 	       list1 (build_unibyte_string ("jansson library not found")));
>   #endif
> 
> and your ensure_json_available use the second part of the above.
> There's no reason to reshuffle the code that works.
> 
> Also, please make ensure_json_available specific to WINDOWSNT, so that
> we don't pay an extra function call on Posix platforms.
> 
> And finally, json--available-p should be defined on all platforms that
> compile json.c, not just on WINDOWSNT; it should return t on Posix
> platforms.

The above is for the emacs-29 branch.  I think on master we should
compile json.c on all platforms, even if libjansson is not being
linked against, and then have json--available-p always fboundp, so
that test at run time will not be needed.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattias.engdegard <at> gmail.com>
Cc: 60311 <at> debbugs.gnu.org
Subject: Re: bug#60311: json-available-p: make dynamically correct for Windows
Date: Sun, 25 Dec 2022 20:43:11 +0200

> From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
> Date: Sun, 25 Dec 2022 15:32:06 +0100
> 
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -6905,11 +6905,11 @@ internal--format-docstring-line
>  
>  (defun json-available-p ()
>    "Return non-nil if Emacs has libjansson support."
> -  (and (fboundp 'json-serialize)
> -       (condition-case nil
> -           (json-serialize t)
> -         (:success t)
> -         (json-unavailable nil))))
> +  (declare (side-effect-free error-free))
> +  (and (eval-when-compile (fboundp 'json-serialize))
> +       ;; If `json--available-p' is present, we need to call it at run-time.
> +       (or (not (eval-when-compile (fboundp 'json--available-p)))
> +           (json--available-p))))

Btw, I don't understand this use of eval-when-compile here.  Can you
explain why should we care at compile time whether these functions are
fboundp?

IOW, why not just

  (and (fboundp 'json--available-p)
       (json--available-p))

?

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

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60311 <at> debbugs.gnu.org
Subject: Re: bug#60311: json-available-p: make dynamically correct for Windows
Date: Mon, 26 Dec 2022 13:11:21 +0100

25 dec. 2022 kl. 16.40 skrev Eli Zaretskii <eliz <at> gnu.org>:

> Yes, you are right.  But please come up with a smaller changeset which
> only changes what strictly needs to be changed.  Or if you want, I can
> do this myself.

Since you have very specific ideas about how to go about it it's better that you make the change yourself. After all, attempts to remote-control someone else this way rarely make either party very happy.
As long as it gets done I'm not very particular about it myself.

>> +  (and (eval-when-compile (fboundp 'json-serialize))
>> +       ;; If `json--available-p' is present, we need to call it at run-time.
>> +       (or (not (eval-when-compile (fboundp 'json--available-p)))
>> +           (json--available-p))))
> 
> Btw, I don't understand this use of eval-when-compile here.  Can you
> explain why should we care at compile time whether these functions are
> fboundp?

Because we can: we already know the answers when subr.el is compiled, giving us constant nil or t values that will simplify the function to a constant or a call to json--available-p.

> why not just
> 
>  (and (fboundp 'json--available-p)
>       (json--available-p))

Calling `fboundp` at run time is wasteful, and that patch used the presence of json--available-p to indicate whether it needs to be called at run time (on Windows only, but it's good to keep that platform-dependency in one place).

It doesn't matter much if json-available-p is implemented in Lisp or C; even performance-wise it's a wash.

Message #28 received at 60311-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattias.engdegard <at> gmail.com>
Cc: 60311-done <at> debbugs.gnu.org
Subject: Re: bug#60311: json-available-p: make dynamically correct for Windows
Date: Mon, 26 Dec 2022 15:29:58 +0200

> From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
> Date: Mon, 26 Dec 2022 13:11:21 +0100
> Cc: 60311 <at> debbugs.gnu.org
> 
> 25 dec. 2022 kl. 16.40 skrev Eli Zaretskii <eliz <at> gnu.org>:
> 
> > Yes, you are right.  But please come up with a smaller changeset which
> > only changes what strictly needs to be changed.  Or if you want, I can
> > do this myself.
> 
> Since you have very specific ideas about how to go about it it's better that you make the change yourself.

Done.

> > why not just
> > 
> >  (and (fboundp 'json--available-p)
> >       (json--available-p))
> 
> Calling `fboundp` at run time is wasteful, and that patch used the presence of json--available-p to indicate whether it needs to be called at run time (on Windows only, but it's good to keep that platform-dependency in one place).

Yes, but using eval-when-compile precludes defining json--available-p
at run time, something that I don't like doing in Emacs, since it's
IMO against the Emacs's spirit of being interpreter based.

So I went with the simpler definition that doesn't assume functions
are only defined at byte-compile time.

Thanks, I'm now closing the bug.

Previous Next

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