Re: Approximates cross-staff slurs in VerticalAxisGroup vertical-skylines. (issue 6498077)

[dak]

http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc
File lily/phrasing-slur-engraver.cc (right):

http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode119
lily/phrasing-slur-engraver.cc:119: if (slur_stubs_.find (slurs_[j]) ==
slur_stubs_.end ())
On 2012/09/02 16:45:14, MikeSol wrote:
> On 2012/09/02 15:59:00, dak wrote:
> > It is quite nonsensical to have slur_stubs be a map indexed via
slurs_[j]
> rather
> > than just an array indexed through j.
> >
> > That is inefficient use of time, memory, and complexity.

> It is sensical because:
> --) It avoids having two vectors (slur_stubs_ and end_slur_stubs_
would be
> needed), thus making the code easier to read and maintain.
> --) It has a "find" method built into it.

I don't see that a "find" method is advantageous over just indexing.
And typical scores _have_ thousands of slurs.

Since you don't bother writing even a single comment, it is rather hard
to figure out what you are doing, and you are doing it in complicated
way.

http://codereview.appspot.com/6498077/diff/21/lily/phrasing-slur-engraver.cc#newcode332
lily/phrasing-slur-engraver.cc:332: map<Grob *, Grob *> vag_to_slur;
On 2012/09/02 16:45:14, MikeSol wrote:
> On 2012/09/02 15:59:00, dak wrote:
> > Again: why a map here?

> find method, see above.
> I know that find exists for vectors in algorithm, but I think this is
easier to
> read and more consistent.

You are apparently going to a lot of effort of getting your map entries
out again when you could just keep arrays with an identical structure to
the slurs_ and end_slurs_ arrays.  Again, since you don't bother writing
any comment regarding what you are trying to do, it is hard to figure
out what the purpose is.

http://codereview.appspot.com/6498077/