Re: Doc: mention empty chords; avoid using zero-duration spacers in examples (issue 6197068)

[graham]

ok.


http://codereview.appspot.com/6197068/diff/24001/Documentation/notation/keyboards.itely
File Documentation/notation/keyboards.itely (right):

http://codereview.appspot.com/6197068/diff/24001/Documentation/notation/keyboards.itely#newcode210
Documentation/notation/keyboards.itely:210: e''2\p\< d''\> | c''1\!
we normally only allow one bar per line unless you're specifically
showing something with lots of bars and have only whole notes.

Unless you want a specific exemption from that policy, could you put the
c1 on the next line?  (also the later c1 and f'1)

http://codereview.appspot.com/6197068/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

PATCH: Countdown to 20120614

[Colin Campbell]

For 20:00 MDT Thursday June 14

Critical:
Issue 2587 
: Ugly slur in 
TabStaff if font is changed - R 6303065 



Defect:
Issue 582 : 
slur and tie start in (almost) same spot - R 6301067 

Issue 2574 
: Footnote on 
first time signature gets mark but no footnote text - R 6306064 



Documentation:
Issue 2522 
: duration*0 
considered harmful - R 6197068 


Enhancement:
Issue 2598 
: Patch: Allow 
brackets and parens to start lyrics, disallow "accents" - R 6297074 

Issue 2596 
: Patch: 
Protect a few events from gc - R 6303066 




Cheers,
Colin

--
I've learned that you shouldn't go through life with a catcher's mitt on both 
hands.
You need to be able to throw something back.
-Maya Angelou, poet (1928- )

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

giving out git push ability

[Graham Percival]

Who thinks they should have git push ability?  I usually tell
people not to ask unless prompted, so now I'm prompting.

General rule of thumb is that you should have a bunch of patches
accepted, and generally be a trustworthy character.  Or something
like that.

- Graham

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[dak]

On 2012/06/12 13:47:34, MikeSol wrote:

On 2012/06/12 13:43:09, dak wrote:



> If a type is named "Interval", it needs to be employed as an

Interval, not as

> something totally different that relies on implementation details.
>
> Otherwise type abstraction makes code _less_ maintainable rather

than more,

> since you always need to take all side-effects into consideration.



Dunno - it sounds like time for clean up.


Maybe.  It does not make sense in my opinion to pepper the whole code
with fixups intended to cater for cases where intervals are supposed to
be used as non-intervals, or with fixups in order to cater for cases
where intervals are supposed to be used as intervals, and we can't put
the respective fixes into the interval implementation itself since it
would violate the assumptions of those uses where intervals are used as
non-intervals.


Y-position of beams were stored as intervals for years (they may still

be - I

forget).


Storing Y-positions of beams in intervals is fine when we are talking
about a possible set of definitions for Y-positions at one point.  When
we are talking about "lower interval bound is left Y-position, higher
interval bound is right Y-position, and left Y-position may be higher
than right Y-position", we are venturing into the domain of nonsense.


This sounds like a pretty major task, so as always, I'd touch base w/

Han-Wen to

see what Intervals were supposed to be at their inception and then

evaluate what

they've grown into.



We can then firm up an Interval API and write a strongly-worded

comment in

interval.hh and interval.tcc NOT to touch either of these files.


Sounds like it would make sense.  And _if_ we have use cases for ordered
point/value pairs (like using Linear_combination or whatever), then we
should make sure that those are available on _appropriate_ data types as
well rather than misusing Intervals for them because their internals
happen to be convenient for that.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

On 2012/06/12 13:43:09, dak wrote:

Here is a code example from beam.cc:



   Interval weights (1 - multiplier, multiplier);



   if (feather_dir != LEFT)
 weights.swap ();



This is _hogwash_.  weights is not an _interval_ here, but a pair of

numbers.

Swapping the bounds of an interval does not even make _sense_.  Where

is the

point in type abstraction if one creates total conceptual chaos

without inherent

relation to the invariants of the mathematical model underlying the

type?


swap never should have been a public member function (it is used

internally when

mirroring intervals).



If a type is named "Interval", it needs to be employed as an Interval,

not as

something totally different that relies on implementation details.



Otherwise type abstraction makes code _less_ maintainable rather than

more,

since you always need to take all side-effects into consideration.


Dunno - it sounds like time for clean up.

Y-position of beams were stored as intervals for years (they may still
be - I forget).

This sounds like a pretty major task, so as always, I'd touch base w/
Han-Wen to see what Intervals were supposed to be at their inception and
then evaluate what they've grown into.

We can then firm up an Interval API and write a strongly-worded comment
in interval.hh and interval.tcc NOT to touch either of these files.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[dak]

Here is a code example from beam.cc:

  Interval weights (1 - multiplier, multiplier);

  if (feather_dir != LEFT)
weights.swap ();

This is _hogwash_.  weights is not an _interval_ here, but a pair of
numbers.  Swapping the bounds of an interval does not even make _sense_.
 Where is the point in type abstraction if one creates total conceptual
chaos without inherent relation to the invariants of the mathematical
model underlying the type?

swap never should have been a public member function (it is used
internally when mirroring intervals).

If a type is named "Interval", it needs to be employed as an Interval,
not as something totally different that relies on implementation
details.

Otherwise type abstraction makes code _less_ maintainable rather than
more, since you always need to take all side-effects into consideration.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

On 2012/06/12 13:22:10, dak wrote:

On 2012/06/12 12:54:40, MikeSol wrote:
> On 2012/06/12 12:49:45, dak wrote:
> > http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc
> > File lily/grob.cc (right):
> >
> >

http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472

> > lily/grob.cc:472: real_ext[d] += offset;
> > On 2012/06/12 12:32:37, dak wrote:
> > > I don't understand this.  The only way to get a nan from adding

an offset

to
> > > infinity is by adding another nan or an infinite offset with

different

sign.
> > >
> > > What case is this supposed to catch?
> >
> > So what you actually meant to say was
> >   if (!real_ext.is_empty ())
> > real_ext.translate (offset);
> >
> > If that's what you mean, why don't you write it instead of some

puzzle?

>
> This is not what I mean.  An empty interval is, in LilyPond, an

interval whose

> left is greater than it's right.



I disagree.  An empty interval is an interval not containing any

point.  The

details are not important.



> So (3 . 2) is empty, but (3 . 2) can be
> translated by an infinite value in either direction and not result

in nan.


What meaning is there in "translating" an "empty" interval and

afterwards

getting an interval no longer being empty since its lower bound no

longer is

smaller than its upper bound?



Are you, by chance, confusing an _interval_ with a tuple of points?

You can

_implement_ a closed interval as a tuple of points, but that does not

lend

meaning to "shifting an empty interval".



> So just checking for emptiness isn't enough.



For mimicking all side effects of the current implementation, it

isn't.  But if

some code relies on those side effects, it is broken in my opinion.

If you want

to manipulate a two-dimensional vector without inherent relation

between its two

elements, use a Drul, not an Interval.



> > And frankly, shouldn't we rather put this into

flower/include/interval.hh

> > instead of trying to catch this in arbitrary places in our code?
>
> I had toyed with this idea - this is a bit out of my league, as I

don't know

if
> LilyPond will take a performance hit if we add extra operations to

something

as
> elemental as translate or is_empty.



Every piece of inherent safety takes a performance hit.  Personally I

think that

the only case where we have is_empty true is for (+inf, -inf).  And I

don't mean

that we should change the test: the test is fine.  There just isn't a
combination of interval bounds that makes more sense.  We could

equally well

define the empty interval as (0, -1) and get consistent behavior from

that.  But

while the interval is empty, its bounds should not be interpreted as

conveying

meaning.


If all empty intervals are the same then, by design, we need to make
sure that all of them equal (+inf.0 . -inf.0).  This means that:

--) Interval should have a method set_to_empty () that sets it to
(+inf.0 . -inf.0) (maybe it already does).
--) Anytime an interval is set to its left being greater than its right
such that it is not infinitely empty, a programming error should be
issued.
--) Perhaps all math done on empty intervals should raise programming
errors.

I'm all for making the code more coherent.  It sounds like we're talking
about a much deeper problem than this patch, and perhaps it's wiser to
come up w/ a solution to that first before pushing this.

Cheers,
MS


http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[dak]

On 2012/06/12 12:54:40, MikeSol wrote:

On 2012/06/12 12:49:45, dak wrote:
> http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc
> File lily/grob.cc (right):
>
>

http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472

> lily/grob.cc:472: real_ext[d] += offset;
> On 2012/06/12 12:32:37, dak wrote:
> > I don't understand this.  The only way to get a nan from adding an

offset to

> > infinity is by adding another nan or an infinite offset with

different sign.

> >
> > What case is this supposed to catch?
>
> So what you actually meant to say was
>   if (!real_ext.is_empty ())
> real_ext.translate (offset);
>
> If that's what you mean, why don't you write it instead of some

puzzle?


This is not what I mean.  An empty interval is, in LilyPond, an

interval whose

left is greater than it's right.


I disagree.  An empty interval is an interval not containing any point.
The details are not important.


So (3 . 2) is empty, but (3 . 2) can be
translated by an infinite value in either direction and not result in

nan.

What meaning is there in "translating" an "empty" interval and
afterwards getting an interval no longer being empty since its lower
bound no longer is smaller than its upper bound?

Are you, by chance, confusing an _interval_ with a tuple of points?  You
can _implement_ a closed interval as a tuple of points, but that does
not lend meaning to "shifting an empty interval".


So just checking for emptiness isn't enough.


For mimicking all side effects of the current implementation, it isn't.
But if some code relies on those side effects, it is broken in my
opinion.  If you want to manipulate a two-dimensional vector without
inherent relation between its two elements, use a Drul, not an Interval.


> And frankly, shouldn't we rather put this into

flower/include/interval.hh

> instead of trying to catch this in arbitrary places in our code?



I had toyed with this idea - this is a bit out of my league, as I

don't know if

LilyPond will take a performance hit if we add extra operations to

something as

elemental as translate or is_empty.


Every piece of inherent safety takes a performance hit.  Personally I
think that the only case where we have is_empty true is for (+inf,
-inf).  And I don't mean that we should change the test: the test is
fine.  There just isn't a combination of interval bounds that makes more
sense.  We could equally well define the empty interval as (0, -1) and
get consistent behavior from that.  But while the interval is empty, its
bounds should not be interpreted as conveying meaning.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

On 2012/06/12 12:49:45, dak wrote:

http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc
File lily/grob.cc (right):



http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472

lily/grob.cc:472: real_ext[d] += offset;
On 2012/06/12 12:32:37, dak wrote:
> I don't understand this.  The only way to get a nan from adding an

offset to

> infinity is by adding another nan or an infinite offset with

different sign.

>
> What case is this supposed to catch?



So what you actually meant to say was
   if (!real_ext.is_empty ())
 real_ext.translate (offset);



If that's what you mean, why don't you write it instead of some

puzzle?

This is not what I mean.  An empty interval is, in LilyPond, an interval
whose left is greater than it's right.  So (3 . 2) is empty, but (3 . 2)
can be translated by an infinite value in either direction and not
result in nan.  So just checking for emptiness isn't enough.

An infinite value in an interval should not be translated by another
infinite value.  It either does nothing (if they both have the same
sign) or results in a nan.


And frankly, shouldn't we rather put this into

flower/include/interval.hh

instead of trying to catch this in arbitrary places in our code?


I had toyed with this idea - this is a bit out of my league, as I don't
know if LilyPond will take a performance hit if we add extra operations
to something as elemental as translate or is_empty.  If this is
appropriate, then the check can go there.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[dak]

http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc
File lily/grob.cc (right):

http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472
lily/grob.cc:472: real_ext[d] += offset;
On 2012/06/12 12:32:37, dak wrote:

I don't understand this.  The only way to get a nan from adding an

offset to

infinity is by adding another nan or an infinite offset with different

sign.


What case is this supposed to catch?


So what you actually meant to say was
  if (!real_ext.is_empty ())
real_ext.translate (offset);

If that's what you mean, why don't you write it instead of some puzzle?

And frankly, shouldn't we rather put this into
flower/include/interval.hh instead of trying to catch this in arbitrary
places in our code?

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[mtsolo]

On 2012/06/12 12:32:37, dak wrote:

http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc
File lily/grob.cc (right):



http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472

lily/grob.cc:472: real_ext[d] += offset;
I don't understand this.  The only way to get a nan from adding an

offset to

infinity is by adding another nan or an infinite offset with different

sign.


What case is this supposed to catch?


Sometimes there is an infinite offset (either positive or negative)
which, when added to an empty interval of (+inf.0 . -inf.0), will create
nan (or -nan) in one of the two directions.

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Sets TabVoice Stem height to ##f (issue 6303065)

[dak]

http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc
File lily/grob.cc (right):

http://codereview.appspot.com/6303065/diff/10003/lily/grob.cc#newcode472
lily/grob.cc:472: real_ext[d] += offset;
I don't understand this.  The only way to get a nan from adding an
offset to infinity is by adding another nan or an infinite offset with
different sign.

What case is this supposed to catch?

http://codereview.appspot.com/6303065/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Deletes logfiles in build dir with make clean (issue 6300079)

[john . mandereau]

I'm not certain cleaning the build tree should include log files
cleaning, when a simple
"find -name '*.log' |xargs rm -f"
does the job.  However, if this is a very popular request, then you may
want to read a few comments.


http://codereview.appspot.com/6300079/diff/1/GNUmakefile.in
File GNUmakefile.in (right):

http://codereview.appspot.com/6300079/diff/1/GNUmakefile.in#newcode340
GNUmakefile.in:340: find . -name "*.log" -delete
I think something like "rm -f *test*.log" is enough here, we don't ask
for removing all "*.log" files of build tree by making test-clean, do
we?

http://codereview.appspot.com/6300079/diff/1/stepmake/stepmake/generic-targets.make
File stepmake/stepmake/generic-targets.make (right):

http://codereview.appspot.com/6300079/diff/1/stepmake/stepmake/generic-targets.make#newcode16
stepmake/stepmake/generic-targets.make:16: find . -name "*.log" -delete
make already recurses into subdirectories, so using find is superfluous,
"rm -f *.log" is enough.  Moreover, this would better go above $(LOOP)
to preserve the logic of cleaning a directory *then* its subdirectories.

http://codereview.appspot.com/6300079/diff/1/stepmake/stepmake/generic-targets.make#newcode209
stepmake/stepmake/generic-targets.make:209: find . -name "*.log" -delete
This line is superfluous, as "$(MAKE) out=www clean" already executes
actions specified in "clean" target.

http://codereview.appspot.com/6300079/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel