bug#69953: [PATCH] Remove duplicated asserts and checks

[Daniel Colascione <dancol@dancol.org>]

Sergey Vinokurov <serg.foo@gmail.com> writes:

> On 09/05/2024 08:24, Eli Zaretskii wrote:
>> tags 69953 wontfix
>> close 69953
>> thanks
>> 
>>> Cc: serg.foo@gmail.com, 69953@debbugs.gnu.org
>>> Date: Sat, 27 Apr 2024 11:27:41 +0300
>>> From: Eli Zaretskii <eliz@gnu.org>
>>>
>>> Ping! Ping!  Philipp and Daniel, any comments?
>>>
>>>> Cc: serg.foo@gmail.com, 69953@debbugs.gnu.org
>>>> Date: Sat, 13 Apr 2024 10:42:50 +0300
>>>> From: Eli Zaretskii <eliz@gnu.org>
>>>>
>>>> Ping! Philipp and Daniel, do you have any comments on this?
>>>>
>>>>> Cc: Philipp Stephani <phst@google.com>, 69953@debbugs.gnu.org,
>>>>>   Daniel Colascione <dancol@dancol.org>
>>>>> Date: Sat, 23 Mar 2024 09:15:04 +0200
>>>>> From: Eli Zaretskii <eliz@gnu.org>
>>>>>
>>>>>> Date: Sat, 23 Mar 2024 03:27:34 +0000
>>>>>> From: Sergey Vinokurov <serg.foo@gmail.com>
>>>>>>
>>>>>> I noticed that emacs-module.c contains duplicate
>>>>>> module_non_local_exit_check() checks and
>>>>>> module_assert_thread/module_assert_env asserts, mostly performed at the
>>>>>> same point in program sequentially.
>>>>>>
>>>>>> The module_non_local_exit_check() checks happen in
>>>>>> MODULE_HANDLE_NONLOCAL_EXIT and MODULE_FUNCTION_BEGIN_NO_CATCH macros.
>>>>>> The MODULE_HANDLE_NONLOCAL_EXIT is never used by itself, only as part of
>>>>>> MODULE_FUNCTION_BEGIN which starts with MODULE_FUNCTION_BEGIN_NO_CATCH
>>>>>> that performs the check.
>>>>>>
>>>>>> In addition, there're 6 "Implementation of runtime and environment
>>>>>> functions" rules outlined where MODULE_HANDLE_NONLOCAL_EXIT should be
>>>>>> called at step 4 but module_non_local_exit_check() is supposed to have
>>>>>> already happened at step 3 so documentation does not seem to intend for
>>>>>> the check to be repeated in MODULE_HANDLE_NONLOCAL_EXIT.
>>>>>>
>>>>>> Regarding asserts my observation is that module_non_local_exit_check()
>>>>>> already contains module_assert_thread and module_assert_env so there's
>>>>>> no need to do asserts if first thing we do is call
>>>>>> module_non_local_exit_check.
>>>>>
>>>>> Thanks, but why is that a problem?  module_assertions is false by
>>>>> default, and the function to turn on module assertions is not even
>>>>> documented in the ELisp manual.  IOW, this is a debugging aid which
>>>>> will rarely if at all activated, and if it is, that's on purpose by
>>>>> the programmer who is investigating some tricky problem.  Why is it a
>>>>> problem to have too many assertions, which might help that programmer
>>>>> find a bug?
>>>>>
>>>>> I added Daniel and Philipp to the discussion, in case they have
>>>>> comments to this proposal.
>> Given the lack of comments, I conclude that there's no interest in
>> installing this, and I'm therefore closing this bug.
>
> I'm interested to get this installed - can I do anything to facilitate?


Programming is not an exercise in Huffman compression :-).  Sometimes,
things that look repetitive are not duplicative: the same text can express
different things in different contexts.  The assertions in this patch
express the intent of the authors of macros like
MODULE_FUNCTION_BEGIN_NO_CATCH.  Their presence in the code signifies
in writing the contract to which users of MODULE_FUNCTION_BEGIN_NO_CATCH
and other facilities must adhere.  Deleting them would therefore make it
incrementally harder to understand the code, as it costs the brain more
to infer a contract than to read one.

I definitely appreciate the desire to clean up the codebase.  I think
the energy that's gone into this thread could be more productively
applied to other code cleanups.