Eli Zaretskii <eliz@gnu.org> schrieb am Di., 24. Nov. 2015 um 20:41 Uhr:

> In "emacs -Q", load the modules/mod-test/mod-test module, then try
> this:
>
>   M-: (mod-test-sum "1" 2) RET
>
> Result: Emacs aborts.  This happens because the eassert in
> module_extract_integer aborts, when that function is called for the
> 2nd time:
>
>   static intmax_t
>   module_extract_integer (emacs_env *env, emacs_value n)
>   {
>     check_main_thread ();
>     eassert (module_non_local_exit_check (env) ==
> emacs_funcall_exit_return);
>     Lisp_Object l = value_to_lisp (n);
>     if (! INTEGERP (l))
>       {
>         module_wrong_type (env, Qintegerp, l);
>         return 0;
>       }
>
> The first call to module_extract_integer correctly detects the wrong
> type of argument and calls module_wrong_type.  But module_wrong_type
> just records the problem in the env structure, it doesn't signal any
> Lisp error, like an Emacs primitive would.  So the actual error goes
> undetected, and is masked by the assertion violation (because Emacs is
> built with --enable-checking).
>
> Since this obviously works as it was designed, my question is: how
> should a module be written so that this kind of errors signal a normal
> Lisp error we are accustomed with Emacs primitives?
>
>
I still need to fix most of the example module (unfortunately that was
written before the error handling got implemented), but the gist is that
functions returning emacs_value should usually be called like this:

emacs_value ret = env->foo(env, ...);
if (ret == NULL) return NULL;

This will cause errors to be reported back to Emacs, where they are thrown
as normal signals.

I have thought about how we can prevent some accidental misuse. I propose
that all environment functions should have a signature like this:

__attribute__((warn_unused_result))
bool (*func)(emacs_env *env, ..., result_type* result);

instead of returning result directly. This way users get a warning if they
forget to check for errors. The above example would then turn into:

emacs_value ret;
if (!env->foo(env, ..., &ret)) return false;

which isn't worse, but avoids sentinel values and makes the API harder to
misuse. Thoughts?