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

[dak]

Reviewers: Dan Eble, benko.pal,

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

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.

Description:
Spacing_spanner::prune_loose_columns: prune in-place

This saves a tiny amount of time and memory.

Please review this at https://codereview.appspot.com/351720043/

Affected files (+14, -9 lines):
  M lily/spacing-determine-loose-columns.cc


Index: lily/spacing-determine-loose-columns.cc
diff --git a/lily/spacing-determine-loose-columns.cc  
b/lily/spacing-determine-loose-columns.cc
index  
4be23042ad44bbddc007c8879714cd0c201bd35d..5b5e994108372844b6cabfcc0b3f5885857e3b58  
100644
--- a/lily/spacing-determine-loose-columns.cc
+++ b/lily/spacing-determine-loose-columns.cc
@@ -189,13 +189,16 @@ Spacing_spanner::prune_loose_columns (Grob *me,
                                       vector<Grob *> *cols,
                                       Spacing_options *options)
 {
-  vector<Grob *> newcols;
-  for (vsize i = 0; i < cols->size (); i++)
+  // In-place iteration will read at rp and conditionally write at wp.
+  vector<Grob *>::const_iterator rp;
+  vector<Grob *>::iterator wp;
+  Grob * lastcol = 0;
+  for (rp = wp = cols->begin (); rp != cols->end ();)
     {
-      Grob *c = cols->at (i);
+      Grob *c = *rp++;

-      bool loose = (i > 0 && i + 1 < cols->size ())
-                   && is_loose_column (cols->at (i - 1), c, cols->at (i +  
1), options);
+      bool loose = (lastcol && rp != cols->end ()
+                    && is_loose_column (lastcol, c, *rp, options));

       /* Breakable columns never get pruned; even if they are loose,
         their broken pieces are not.  However, we mark them so that
@@ -230,8 +233,8 @@ Spacing_spanner::prune_loose_columns (Grob *me,
           if (!right_neighbor || !left_neighbor)
             {
               c->programming_error ("Cannot determine neighbors for  
floating column.");
-              c->set_object ("between-cols", scm_cons (cols->at (i -  
1)->self_scm (),
-                                                       cols->at (i +  
1)->self_scm ()));
+              c->set_object ("between-cols", scm_cons (lastcol->self_scm  
(),
+                                                       (*rp)->self_scm  
()));
             }
           else
             {
@@ -249,10 +252,12 @@ Spacing_spanner::prune_loose_columns (Grob *me,
         }

       else
-        newcols.push_back (c);
+        *wp++ = c;
+
+      lastcol = c;
     }

-  *cols = newcols;
+  cols->erase (wp, rp);
 }

 /*