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

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gerd Möllmann <gerd.moellmann <at> gmail.com>
Cc: monnier <at> iro.umontreal.ca, 77924 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: bug#77924: 31.0.50; [Feature branch] Change marker implementation
Date: Fri, 25 Apr 2025 09:21:11 +0300

> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Cc: monnier <at> iro.umontreal.ca,  stefankangas <at> gmail.com,  77924 <at> debbugs.gnu.org
> Date: Thu, 24 Apr 2025 18:27:51 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> -  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.

These implementation details are not important for someone who wants
to understand what the call does, as opposed to someone who studies
the implementation.  So having marker_position etc. will make it
easier to read the code, let alone get us shorter names.

> >> +  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.

It would mean that each loop will need to reproduce some of the
boilerplate code you now hide behind DO_MARKERS, such that the opening
braces are not part of the macro, but are explicit in the source.
Like we do in the FOR_EACH_* macros.  That would require some more
typing, but the benefits IMO far outweigh this downside.

> 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.

Thanks for working on this in the first place.

Previous Next

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