Issue 4577: Remove the nasty workaround for platforms that don't provide std::vector::data().

Remove the nasty workaround for platforms that don't provide
std::vector::data().

Replace most code that reached into into Grob_array and modified its
internal vector with calls to new member functions such as filter().

Move the definitions of some trivial methods into grob-array.hh.

[dak, 2015-08-29 07:33:19]

"Beautify" is appropriate for changes in comments or formatting.  For code, it
is subjective and non-descriptive.  Overall, the changes look like an
improvement.

https://codereview.appspot.com/264950043/diff/1/lily/break-substitution.cc
File lily/break-substitution.cc (right):

https://codereview.appspot.com/264950043/diff/1/lily/break-substitution.cc#ne...
lily/break-substitution.cc:446: orig->filter_map_into(substitute_grob,
*new_arr);
Coding style mishap, missing space: filter_map_into (

https://codereview.appspot.com/264950043/diff/1/lily/break-substitution.cc#ne...
lily/break-substitution.cc:492: grob_array->filter_map_into(substitute_grob,
*new_arr);
Coding style.

https://codereview.appspot.com/264950043/diff/1/lily/grob-array.cc
File lily/grob-array.cc (right):

https://codereview.appspot.com/264950043/diff/1/lily/grob-array.cc#newcode88
lily/grob-array.cc:88: // could call grobs_.shrink_to_fit() with C++11
Coding style: shrink_to_fit ()

https://codereview.appspot.com/264950043/diff/1/lily/grob-array.cc#newcode99
lily/grob-array.cc:99: // could call grobs_.shrink_to_fit() with C++11
Coding style.

https://codereview.appspot.com/264950043/diff/1/lily/grob-array.cc#newcode102
lily/grob-array.cc:102: // Like filter_map(f), but store the result in the given
array.
Coding style.

https://codereview.appspot.com/264950043/diff/1/lily/grob-array.cc#newcode105
lily/grob-array.cc:105: Grob_array &result) const
Conceptually this is not a member function, at least not of the _source_
Grob_array.  It's more of an assignment/initialization.  So it's more of a

Grob_array &
Grob_array::filter_map_assign (Grob * (*map_fun) (Grob*), const Grob_array
&src);

https://codereview.appspot.com/264950043/diff/1/lily/grob-array.cc#newcode114
lily/grob-array.cc:114: // could call result.grobs_.shrink_to_fit() with C++11
Coding style.

https://codereview.appspot.com/264950043/diff/1/lily/include/grob-array.hh
File lily/include/grob-array.hh (right):

https://codereview.appspot.com/264950043/diff/1/lily/include/grob-array.hh#ne...
lily/include/grob-array.hh:64: // set_ordered(), which could lead to unexpected
behavior.
Coding style.

https://codereview.appspot.com/264950043/diff/1/lily/include/grob-array.hh#ne...
lily/include/grob-array.hh:67: // Like filter_map(f), but store the result in
the given array.
Coding style.

https://codereview.appspot.com/264950043/diff/1/lily/include/grob-array.hh#ne...
lily/include/grob-array.hh:70: // set_ordered(), which could lead to unexpected
behavior.
Coding style.

https://codereview.appspot.com/264950043/diff/1/lily/pure-from-neighbor-inter...
File lily/pure-from-neighbor-interface.cc (right):

https://codereview.appspot.com/264950043/diff/1/lily/pure-from-neighbor-inter...
lily/pure-from-neighbor-interface.cc:45: a->set_array(new_elts);
Coding style.

https://codereview.appspot.com/264950043/diff/1/lily/slur.cc
File lily/slur.cc (right):

https://codereview.appspot.com/264950043/diff/1/lily/slur.cc#newcode223
lily/slur.cc:223: a->set_array(new_encompasses);
Coding style.

[pkx166h, 2015-08-29 07:43:37]

Passes make, make check and a full make doc.

PATCH_REVIEW

[Dan Eble, 2015-08-29 13:11:02]

whitespace and comments

[Dan Eble, 2015-08-29 13:12:19]

On 2015/08/29 13:11:02, Dan Eble wrote:
> whitespace and comments

I'll address filter_map_into/filter_map_assign later; I'm not sure if I'll have
time this weekend.

[Dan Eble, 2015-09-01 02:30:20]

filter_map_into -> filter_map_assign

[Dan Eble, 2015-09-01 02:31:52]

On 2015/09/01 02:30:20, Dan Eble wrote:
> filter_map_into -> filter_map_assign

PATCH_NEW

[benko.pal, 2015-09-02 10:13:26]

https://codereview.appspot.com/264950043/diff/40001/lily/include/grob-array.hh
File lily/include/grob-array.hh (right):

https://codereview.appspot.com/264950043/diff/40001/lily/include/grob-array.h...
lily/include/grob-array.hh:63: // Note: This method may reorder the array
without affecting the result of
what does "reorder" mean in the current context?  the function replaces some
grobs, deletes others.

[Dan Eble, 2015-09-02 23:27:41]

On 2015/09/02 10:13:26, benko.pal wrote:
>
https://codereview.appspot.com/264950043/diff/40001/lily/include/grob-array.h...
> lily/include/grob-array.hh:63: // Note: This method may reorder the array
> without affecting the result of
> what does "reorder" mean in the current context?  the function replaces some
> grobs, deletes others.

If the sorting criterion uses attributes of the Grob that are changed by
replacement, the array might not be sorted afterward (or might be sorted when it
wasn't before).  Grob_array does not appear to dictate the sorting criterion.

[dak, 2015-09-03 08:49:13]

On 2015/09/02 23:27:41, Dan Eble wrote:

> If the sorting criterion uses attributes of the Grob that are changed by
> replacement, the array might not be sorted afterward (or might be sorted when
it
> wasn't before).  Grob_array does not appear to dictate the sorting criterion.

So the routine should reset the "sorted" flag when any changes are made?

[Dan Eble, 2015-09-05 13:08:02]

On 2015/09/03 08:49:13, dak wrote:
> On 2015/09/02 23:27:41, Dan Eble wrote:
> 
> > If the sorting criterion uses attributes of the Grob that are changed by
> > replacement, the array might not be sorted afterward (or might be sorted
when
> it
> > wasn't before).  Grob_array does not appear to dictate the sorting
criterion.
> 
> So the routine should reset the "sorted" flag when any changes are made?

Maybe; but it didn't before, and I don't want to deal with the consequences of
changing the logic.  With your consent, I'd like to move the ticket along to
"countdown."

[Dan Eble, 2015-09-05 13:11:23]

On 2015/09/05 13:08:02, Dan Eble wrote:
> On 2015/09/03 08:49:13, dak wrote:
> > On 2015/09/02 23:27:41, Dan Eble wrote:
> > 
> > > If the sorting criterion uses attributes of the Grob that are changed by
> > > replacement, the array might not be sorted afterward (or might be sorted
> when
> > it
> > > wasn't before).  Grob_array does not appear to dictate the sorting
> criterion.
> > 
> > So the routine should reset the "sorted" flag when any changes are made?
> 
> Maybe; but it didn't before, and I don't want to deal with the consequences of
> changing the logic.  With your consent, I'd like to move the ticket along to
> "countdown."

Correction: I'd like your consent to push it.  It was already listed in
"Countdown" in James' email of Sep. 2, but the ticket was out of date.

[dak, 2015-09-05 13:45:35]

On 2015/09/05 13:11:23, Dan Eble wrote:
> On 2015/09/05 13:08:02, Dan Eble wrote:
> > On 2015/09/03 08:49:13, dak wrote:
> > > On 2015/09/02 23:27:41, Dan Eble wrote:
> > > 
> > > > If the sorting criterion uses attributes of the Grob that are changed by
> > > > replacement, the array might not be sorted afterward (or might be sorted
> > when
> > > it
> > > > wasn't before).  Grob_array does not appear to dictate the sorting
> > criterion.
> > > 
> > > So the routine should reset the "sorted" flag when any changes are made?

Correction, "ordered_".

> > 
> > Maybe; but it didn't before, and I don't want to deal with the consequences
of
> > changing the logic.  With your consent, I'd like to move the ticket along to
> > "countdown."
> 
> Correction: I'd like your consent to push it.  It was already listed in
> "Countdown" in James' email of Sep. 2, but the ticket was out of date.

You are replacing one-shot code with general facilities here.  If the
circumstances of the one-shot code happen not to trigger a particular problem in
a visible manner, that does not mean that this will hold the same for other
future uses of the facility.  So it's better to make sure that a routine does
what it claims to do under its claimed preconditions now while it is fresh.  It
will be much more troublesome to figure out the culprit when this indeed should
become a problem eventually and the code is "long-standing".

[Keith, 2015-09-05 17:13:55]

On 2015/09/03 08:49:13, dak wrote:
> On 2015/09/02 23:27:41, Dan Eble wrote:
> 
> > If the sorting criterion uses attributes of the Grob that are changed by
> > replacement, the array might not be sorted afterward (or might be sorted
when
> it
> > wasn't before).  Grob_array does not appear to dictate the sorting
criterion.

The flag 'ordered_' is used to protect an array from re-arrangement, such as the
order of staves in a system, rather than to report whether the array has been
sorted.  See its only use at
https://codereview.appspot.com/264950043/diff/40001/lily/break-substitution.c...
where it prevents an optimization that would otherwise re-order the array.

If filter_map() does change the conceptual order in this way, then it may not be
used on an array marked as ordered_.

> 
> So the routine should reset the "sorted" flag when any changes are made?

In this case, the converse:  If 'ordered_' is set, no changes may be made.
The safer, more conservative, setting of 'ordered_' is 'true'.

[Dan Eble, 2015-09-05 20:37:12]

On 2015/09/05 17:13:55, Keith wrote:
> 
> In this case, the converse:  If 'ordered_' is set, no changes may be made.
> The safer, more conservative, setting of 'ordered_' is 'true'.

Hm.  Thanks for the observation.  Then there should be two classes of grob
array, one of which lacks the methods that would reorder the elements.  But to
limit the scope of this patch, I'll just assert(not ordered) in the new
functions.

[Dan Eble, 2015-09-06 00:30:24]

rebase to current master

[Dan Eble, 2015-09-06 00:45:25]

remove cautionary comments

[Dan Eble, 2015-09-06 00:49:44]

On 2015/09/06 00:45:25, Dan Eble wrote:
> remove cautionary comments

Forbidding filtering of "ordered" grob arrays causes make check to abort.  I
have simply removed the comments warning about filtering ordered arrays.  I am
now burned out on this task (and the avoidance of "using namespace std" that is
blocked by it), so this change will sit around until either the reviewers change
their minds about it or my interest in working on it returns. (Understand I'm
not ungrateful for the feedback, just tired.)

[Keith, 2015-09-06 01:50:43]

On Sat, 05 Sep 2015 17:49:44 -0700, <nine.fierce.ballads@gmail.com> wrote:

> I have simply removed the comments warning about filtering
> ordered arrays.

That seems to be correct.  Those comments were incorrect.
The filter_map* functions do not, in fact, change the order of the array under
the meaning of 'ordered_'.  Pál's comment makes sense.

There is no sorting method in Grob_array so there is no reason to suspect that
the member 'ordered_' has anything to do with order under a sorting criterion. 
Looking at the commits that touch 'ordered_' it is clear that its meaning was
"this array should stay in its current order" and that removal of elements
doesn't count as a change in order and substituting Grobs inherit the order the
Grobs they replace.

[dak, 2015-09-06 07:25:12]

On 2015/09/06 01:50:43, Keith wrote:
> On Sat, 05 Sep 2015 17:49:44 -0700, <mailto:nine.fierce.ballads@gmail.com>
wrote:
> 
> > I have simply removed the comments warning about filtering
> > ordered arrays.
> 
> That seems to be correct.  Those comments were incorrect.
> The filter_map* functions do not, in fact, change the order of the array under
> the meaning of 'ordered_'.  Pál's comment makes sense.
> 
> There is no sorting method in Grob_array so there is no reason to suspect that
> the member 'ordered_' has anything to do with order under a sorting criterion.

> Looking at the commits that touch 'ordered_' it is clear that its meaning was
> "this array should stay in its current order" and that removal of elements
> doesn't count as a change in order and substituting Grobs inherit the order
the
> Grobs they replace.

Thanks for figuring this out, and sorry to Dan for the false alarm.  I wish our
codebase were less writeonly so that one could readily guess this from the
comments in the respective header files.

grob-array.hh and grob-array.cc combined contain a single comment apart from the
copyright header, namely

#if 0  /* see System::derived_mark () const */

which is cryptic in itself.  It also contains

void
Grob_array::remove_duplicates ()
{
  assert (!ordered_);

  uniquify (grobs_);
}

which I am less that sure even makes sense any more: since issue 3365, the
variant of uniquify employed here preserves order anyway.  I probably was not
really capable of figuring out with reasonable effort what this assert was
supposed to protect against and rather left it in.

It takes far too much reverse engineering to work on LilyPond.