Re: Spacing_spanner::prune_loose_columns: prune in-place (issue 351720043 by address@hidden)

[benko . pal]

On 2018/07/10 18:09:19, dak wrote:
> On 2018/07/10 17:49:33, dak wrote:
> > On 2018/07/10 17:10:53, benko.pal wrote:
> > > LGTM; just by looking I can't see how it can make make fail.
> > > using rp[-2] and rp[-1] instead of lastcol and c would be cleaner
to me, but
> > > YMMV.
> >
> > Since the whole point is to do the operation in-place, rp[-2] may
already have
> > been overwritten by rp[-1] in the last iteration.  It may seem
cleaner to you
> to
> > use rp[-2] but it would be a rather ugly bug.  Doing things in-place
is more
> > efficient but you have to keep track of what you are doing.

ah yes, thanks for pointing this out!
actually this may worth some comments; I mean, documenting why looseness
decision is based only on original neighbours, not on after-pruning
neighbours (I know rp[-2] may not be the after-pruning neighbour
either).  this may be obvious, but I feel uneasy by just reading this
file.

> > I consider it possible that mixing a const iterator with a non-const
one might
> > be what caused the compilation error but I'd want to see the error
message to
> > be sure.

> Just seen <https://stackoverflow.com/a/12662703> which indicates that
as of
> C++11, you can use a const_iterator for a call to erase (since the
modifiability
> is ensured by the object erase is called on and the const-ness of the
iterator
> is irrelevant) but apparently not previously.  So I guess it is my
newer
> compiler that made the erase call work without complaint.  I'll see
how I can
> make this C++08(?)-save.

that was my guess too, but while I messed with git and make, you beat me
:)
looks still good to me.

https://codereview.appspot.com/351720043/