GNU bug report logs - #69953
[PATCH] Remove duplicated asserts and checks

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 69953 in the body.
You can then email your comments to 69953 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: Sergey Vinokurov <serg.foo <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Remove duplicated asserts and checks
Date: Sat, 23 Mar 2024 03:27:34 +0000

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

Hello,

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.

Regards,
Sergey

[0001-Remove-duplicated-asserts-and-checks.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sergey Vinokurov <serg.foo <at> gmail.com>
Cc: Philipp Stephani <phst <at> google.com>, 69953 <at> debbugs.gnu.org,
 Daniel Colascione <dancol <at> dancol.org>
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Sat, 23 Mar 2024 09:15:04 +0200

> Date: Sat, 23 Mar 2024 03:27:34 +0000
> From: Sergey Vinokurov <serg.foo <at> 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.

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

From: Sergey Vinokurov <serg.foo <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Philipp Stephani <phst <at> google.com>, 69953 <at> debbugs.gnu.org,
 Daniel Colascione <dancol <at> dancol.org>
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Sat, 23 Mar 2024 12:38:37 +0000

The issue is that the same thing is being checked just after it was 
checked. The checks I propose to remove are not going to catch any new 
errors under any possible scenario.

Even with module_assertions disabled, the assert functions are still 
called - a very minor annoyance.

On 23/03/2024 07:15, Eli Zaretskii wrote:
>> Date: Sat, 23 Mar 2024 03:27:34 +0000
>> From: Sergey Vinokurov <serg.foo <at> 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.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: phst <at> google.com, dancol <at> dancol.org
Cc: serg.foo <at> gmail.com, 69953 <at> debbugs.gnu.org
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Sat, 13 Apr 2024 10:42:50 +0300

Ping! Philipp and Daniel, do you have any comments on this?

> Cc: Philipp Stephani <phst <at> google.com>, 69953 <at> debbugs.gnu.org,
>  Daniel Colascione <dancol <at> dancol.org>
> Date: Sat, 23 Mar 2024 09:15:04 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > Date: Sat, 23 Mar 2024 03:27:34 +0000
> > From: Sergey Vinokurov <serg.foo <at> 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.
> 
> 
> 
> 

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philipp Stephani <p.stephani2 <at> gmail.com>, dancol <at> dancol.org
Cc: serg.foo <at> gmail.com, 69953 <at> debbugs.gnu.org
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Sat, 27 Apr 2024 11:27:41 +0300

Ping! Ping!  Philipp and Daniel, any comments?

> Cc: serg.foo <at> gmail.com, 69953 <at> debbugs.gnu.org
> Date: Sat, 13 Apr 2024 10:42:50 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> Ping! Philipp and Daniel, do you have any comments on this?
> 
> > Cc: Philipp Stephani <phst <at> google.com>, 69953 <at> debbugs.gnu.org,
> >  Daniel Colascione <dancol <at> dancol.org>
> > Date: Sat, 23 Mar 2024 09:15:04 +0200
> > From: Eli Zaretskii <eliz <at> gnu.org>
> > 
> > > Date: Sat, 23 Mar 2024 03:27:34 +0000
> > > From: Sergey Vinokurov <serg.foo <at> 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.
> > 
> > 
> > 
> > 
> 
> 
> 
> 

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: p.stephani2 <at> gmail.com, dancol <at> dancol.org, serg.foo <at> gmail.com
Cc: 69953 <at> debbugs.gnu.org
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Thu, 09 May 2024 10:24:56 +0300

tags 69953 wontfix
close 69953
thanks

> Cc: serg.foo <at> gmail.com, 69953 <at> debbugs.gnu.org
> Date: Sat, 27 Apr 2024 11:27:41 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> Ping! Ping!  Philipp and Daniel, any comments?
> 
> > Cc: serg.foo <at> gmail.com, 69953 <at> debbugs.gnu.org
> > Date: Sat, 13 Apr 2024 10:42:50 +0300
> > From: Eli Zaretskii <eliz <at> gnu.org>
> > 
> > Ping! Philipp and Daniel, do you have any comments on this?
> > 
> > > Cc: Philipp Stephani <phst <at> google.com>, 69953 <at> debbugs.gnu.org,
> > >  Daniel Colascione <dancol <at> dancol.org>
> > > Date: Sat, 23 Mar 2024 09:15:04 +0200
> > > From: Eli Zaretskii <eliz <at> gnu.org>
> > > 
> > > > Date: Sat, 23 Mar 2024 03:27:34 +0000
> > > > From: Sergey Vinokurov <serg.foo <at> 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.

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

From: Sergey Vinokurov <serg.foo <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, p.stephani2 <at> gmail.com, dancol <at> dancol.org
Cc: 69953 <at> debbugs.gnu.org
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Thu, 9 May 2024 15:16:05 +0100

On 09/05/2024 08:24, Eli Zaretskii wrote:
> tags 69953 wontfix
> close 69953
> thanks
> 
>> Cc: serg.foo <at> gmail.com, 69953 <at> debbugs.gnu.org
>> Date: Sat, 27 Apr 2024 11:27:41 +0300
>> From: Eli Zaretskii <eliz <at> gnu.org>
>>
>> Ping! Ping!  Philipp and Daniel, any comments?
>>
>>> Cc: serg.foo <at> gmail.com, 69953 <at> debbugs.gnu.org
>>> Date: Sat, 13 Apr 2024 10:42:50 +0300
>>> From: Eli Zaretskii <eliz <at> gnu.org>
>>>
>>> Ping! Philipp and Daniel, do you have any comments on this?
>>>
>>>> Cc: Philipp Stephani <phst <at> google.com>, 69953 <at> debbugs.gnu.org,
>>>>   Daniel Colascione <dancol <at> dancol.org>
>>>> Date: Sat, 23 Mar 2024 09:15:04 +0200
>>>> From: Eli Zaretskii <eliz <at> gnu.org>
>>>>
>>>>> Date: Sat, 23 Mar 2024 03:27:34 +0000
>>>>> From: Sergey Vinokurov <serg.foo <at> 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?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sergey Vinokurov <serg.foo <at> gmail.com>
Cc: p.stephani2 <at> gmail.com, dancol <at> dancol.org, 69953 <at> debbugs.gnu.org
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Sat, 11 May 2024 13:02:01 +0300

> Date: Thu, 9 May 2024 15:16:05 +0100
> Cc: 69953 <at> debbugs.gnu.org
> From: Sergey Vinokurov <serg.foo <at> gmail.com>
> 
> >>>> 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?

Given the lack of responses, I doubt that.

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

From: Sergey Vinokurov <serg.foo <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: p.stephani2 <at> gmail.com, dancol <at> dancol.org, 69953 <at> debbugs.gnu.org
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Sat, 11 May 2024 13:12:33 +0100

On 11/05/2024 11:02, Eli Zaretskii wrote:
>> Date: Thu, 9 May 2024 15:16:05 +0100
>> Cc: 69953 <at> debbugs.gnu.org
>> From: Sergey Vinokurov <serg.foo <at> gmail.com>
>>
>>>>>> 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?
> 
> Given the lack of responses, I doubt that.

Is there no way to reevaluate the patch from the start? It's fairly 
small, perhaps I did poor job explaining why it's good idea?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sergey Vinokurov <serg.foo <at> gmail.com>
Cc: p.stephani2 <at> gmail.com, dancol <at> dancol.org, 69953 <at> debbugs.gnu.org
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Sat, 11 May 2024 15:18:32 +0300

> Date: Sat, 11 May 2024 13:12:33 +0100
> Cc: p.stephani2 <at> gmail.com, dancol <at> dancol.org, 69953 <at> debbugs.gnu.org
> From: Sergey Vinokurov <serg.foo <at> gmail.com>
> 
> On 11/05/2024 11:02, Eli Zaretskii wrote:
> >> Date: Thu, 9 May 2024 15:16:05 +0100
> >> Cc: 69953 <at> debbugs.gnu.org
> >> From: Sergey Vinokurov <serg.foo <at> gmail.com>
> >>
> >>>>>> 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?
> > 
> > Given the lack of responses, I doubt that.
> 
> Is there no way to reevaluate the patch from the start? It's fairly 
> small, perhaps I did poor job explaining why it's good idea?

AFAIU, the code isn't wrong, and doesn't cause any problems.  You just
think it's redundant.  That's a judgment call, and without Daniel and
Philipp, who worked on most of this case, assessing the proposal, I
don't want to change code that worked well for us for several Emacs
releases, just because it might be redundant.

Why are you so eager to install these changes?

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

From: Sergey Vinokurov <serg.foo <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: p.stephani2 <at> gmail.com, dancol <at> dancol.org, 69953 <at> debbugs.gnu.org
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Sat, 11 May 2024 13:57:31 +0100

On 11/05/2024 13:18, Eli Zaretskii wrote:
>> Date: Sat, 11 May 2024 13:12:33 +0100
>> Cc: p.stephani2 <at> gmail.com, dancol <at> dancol.org, 69953 <at> debbugs.gnu.org
>> From: Sergey Vinokurov <serg.foo <at> gmail.com>
>>
>>> Given the lack of responses, I doubt that.
>>
>> Is there no way to reevaluate the patch from the start? It's fairly
>> small, perhaps I did poor job explaining why it's good idea?
> 
> AFAIU, the code isn't wrong, and doesn't cause any problems.  You just
> think it's redundant.  That's a judgment call, and without Daniel and
> Philipp, who worked on most of this case, assessing the proposal, I
> don't want to change code that worked well for us for several Emacs
> releases, just because it might be redundant.
> 
> Why are you so eager to install these changes?

As I benefit from Emacs daily I want to help make it the best version of 
itself. Many things in Emacs I rely on were made by someone, I thought 
this change would ever so slightly benefit others.

It's small indeed and maybe even inconsequential. But as the saying 
goes, the devil is in the details - the small things. Another reason to 
bother is to see whether I can manage to get small things merged - 
bigger contributions would likely involve all the hard parts of merging 
small contributions and then some. If I'm not able to finish small 
things then bigger ones are unlikely to succeed.

As for judgement call I don't know, I mostly unwrapped the various macro 
calls and saw that what one ends up with is assert after assert of the 
same property (e.g. that current thread is OK). For example, if in plain 
C within Emacs, in some other module one found

Lisp_Object
foo(Lisp_Object arg)
  {
    ...
    eassert(NILP(arg));
    eassert(NILP(arg));
    ...
  }

then it would surely be desirable to keep only one call to eassert as 
the second one serves no purpose and provides no extra checks. I argue 
that my patch does exactly that, the only complication is that some 
assert calls come from macro definitions which need to be unwrapped to 
see what's going on.

To paraphrase the example, I removed the second call to assert while 
keeping the first one so same checks happen in the same places as before.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sergey Vinokurov <serg.foo <at> gmail.com>
Cc: p.stephani2 <at> gmail.com, dancol <at> dancol.org, 69953 <at> debbugs.gnu.org
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Sat, 11 May 2024 16:05:15 +0300

> Date: Sat, 11 May 2024 13:57:31 +0100
> Cc: p.stephani2 <at> gmail.com, dancol <at> dancol.org, 69953 <at> debbugs.gnu.org
> From: Sergey Vinokurov <serg.foo <at> gmail.com>
> 
> It's small indeed and maybe even inconsequential. But as the saying 
> goes, the devil is in the details - the small things. Another reason to 
> bother is to see whether I can manage to get small things merged - 
> bigger contributions would likely involve all the hard parts of merging 
> small contributions and then some. If I'm not able to finish small 
> things then bigger ones are unlikely to succeed.

I'm sure you will find a lot of other opportunities for small
contributions, no need to extrapolate from this one.

In many places in Emacs, there's more to the code than meets the eye,
so bitter experience taught me to be extremely cautious with code I'm
not familiar enough.

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

From: Daniel Colascione <dancol <at> dancol.org>
To: Sergey Vinokurov <serg.foo <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 69953 <at> debbugs.gnu.org, p.stephani2 <at> gmail.com
Subject: Re: bug#69953: [PATCH] Remove duplicated asserts and checks
Date: Sat, 11 May 2024 06:11:01 -0700

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

> On 09/05/2024 08:24, Eli Zaretskii wrote:
>> tags 69953 wontfix
>> close 69953
>> thanks
>> 
>>> Cc: serg.foo <at> gmail.com, 69953 <at> debbugs.gnu.org
>>> Date: Sat, 27 Apr 2024 11:27:41 +0300
>>> From: Eli Zaretskii <eliz <at> gnu.org>
>>>
>>> Ping! Ping!  Philipp and Daniel, any comments?
>>>
>>>> Cc: serg.foo <at> gmail.com, 69953 <at> debbugs.gnu.org
>>>> Date: Sat, 13 Apr 2024 10:42:50 +0300
>>>> From: Eli Zaretskii <eliz <at> gnu.org>
>>>>
>>>> Ping! Philipp and Daniel, do you have any comments on this?
>>>>
>>>>> Cc: Philipp Stephani <phst <at> google.com>, 69953 <at> debbugs.gnu.org,
>>>>>   Daniel Colascione <dancol <at> dancol.org>
>>>>> Date: Sat, 23 Mar 2024 09:15:04 +0200
>>>>> From: Eli Zaretskii <eliz <at> gnu.org>
>>>>>
>>>>>> Date: Sat, 23 Mar 2024 03:27:34 +0000
>>>>>> From: Sergey Vinokurov <serg.foo <at> 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.

Previous Next

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