GNU bug report logs - #77924
31.0.50; [Feature branch] Change marker implementation

Previous Next

Full log

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

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: monnier <at> iro.umontreal.ca, 77924 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#77924: 31.0.50; [Feature branch] Change marker implementation
Date: Thu, 24 Apr 2025 18:27:51 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: gerd.moellmann <at> gmail.com, stefankangas <at> gmail.com, 77924 <at> debbugs.gnu.org
>> Date: Wed, 23 Apr 2025 17:41:18 +0300
>> From: Eli Zaretskii <eliz <at> gnu.org>
>>
>> > From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>> > Cc: Eli Zaretskii <eliz <at> gnu.org>,  stefankangas <at> gmail.com,
>> >   77924 <at> debbugs.gnu.org
>> > Date: Wed, 23 Apr 2025 10:34:11 -0400
>> >
>> > FWIW, I'm not a great fan of rebasing either.  I did rebase the branch
>> > last night not for rebasing's sake but because I felt there was a need
>> > for more detailed commit messages.
>> >
>> > In any case, any objection to merging the branch?
>>
>> I'd like to have a closer review of it first, so please wait for a
>> while.  When I skimmed it, I remember having several, hopefully minor,
>> aspects that stood out, and I want to take a closer look.
>
> See below:
>
>> -  m->next = BUF_MARKERS (buf);
>> -  BUF_MARKERS (buf) = m;
>> +  marker_vector_add (buf, m);
>> +  marker_vector_set_charpos (m, charpos);
>
> Could we please name functions that manipulate and get marker
> information marker_SOMETHING and not marker_vector_something?
> marker_vector_add is semi-okay (although marker_add would have been
> nicer, IMO), but marker_vector_set_charpos and marker_vector_charpos
> are not, because AFAIU they manipulate the marker's position, not
> marker-vector's position.
>
> In general, wherever the fact that we have a vector of markers is
> merely an implementation detail that is not important to the
> programmer, can we just drop the "vector" part from the names of the
> functions, for the same reason that the old functions and macros
> weren't called marker_list_SOMETHING?

Well. For one thing, the marker is now no longer the central data
structure. It's basically only fancy index into a marker vector + 2 bit
flags (which could/should by moved to the marker vector, maybe). The
marker vector is the central data structure holding a character position
which the marker refers to.

Secondly functions marker_position and marker_byte_position still exist,
basically one level higher. But below that level, the implementation
necessarily lurks.

That's why I'd prefer to keep the marker_vector prefix.

>> +  DO_MARKERS (b, m)
>> +    {
>> +      if (!vectorlike_marked_p (&m->header))
>> +	marker_vector_remove (XVECTOR (BUF_MARKERS (b)), m);
>> +    }
>> +  END_DO_MARKERS;
>
> Would it be possible not to introduce macros that modify the C syntax?
> These are problematic for more than one reason (one being that they
> are not recognized by C modes we use).  We have macros whose names
> start with FOR_EACH_ (like FOR_EACH_FRAME); can we introduce
> FOR_EACH_MARKER instead, and not hide opening/closing braces inside
> macros?

I tried, already in igc, but failed. IOW, I don't now a way to get rid
of the END_DO_MARKERS.

[...]

I hope Stef can take care of some of the rest. Stef, please let me know
if should help. It could take a few days though, because of real-world
interference. 

Thanks for the review.

Previous Next

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