Re: PartCombine: Keep track of the state in the Part_combine_engraver (issue3334043)

[percival . music . ca]

Nothing obviously wrong in a regtest comparison, and the code style
looks fine to me.


http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc
File lily/part-combine-iterator.cc (right):

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#newcode69
lily/part-combine-iterator.cc:69: Stream_event *unisono_event_;
On 2011/01/25 17:11:19, Reinhold wrote:
> On 2010/11/26 23:05:51, Valentin Villenave wrote:
> > How about declaring these variables on the same line?

> I don't think that's our coding style.

I agree; let's keep the variable declarations separate.  They can be
grouped by adding extra newlines if necessary (as they are already).

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#newcode110
lily/part-combine-iterator.cc:110: void broadcast_new_state
(Music_iterator *new_active, Stream_event *new_state_event);
On 2011/01/25 17:11:19, Reinhold wrote:
> On 2010/11/26 23:05:51, Valentin Villenave wrote:
> > I'm not familiar with C++ coding style, but that looks like an
awfully long
> > line.

> I can shorten the argument names, that would save a few characters...

I think it's clearer to read them as is.  I wouldn't call that a really
long line.  87 chars is getting a bit uncomfortable, but it's not
terrible.  Other places in our code go over by more.

If any change were to be made here, I'd suggest a newline before
"Stream_event".

http://codereview.appspot.com/3334043/diff/1/lily/part-combine-iterator.cc#newcode110
lily/part-combine-iterator.cc:110: void broadcast_new_state
(Music_iterator *new_active, Stream_event *new_state_event);
On 2011/01/25 17:11:19, Reinhold wrote:
> On 2010/11/26 23:05:51, Valentin Villenave wrote:
> > I'm not familiar with C++ coding style, but that looks like an
awfully long
> > line.

> I can shorten the argument names, that would save a few characters...

I think it's clearer to read them as is.  I wouldn't call that a really
long line.  87 chars is getting a bit uncomfortable, but it's not
terrible.  Other places in our code go over by more.

If any change were to be made here, I'd suggest a newline before
"Stream_event".

http://codereview.appspot.com/3334043/