Does type checking for all context property sets.

Does type checking for all context property sets.

[MikeSol, 2011-07-06 07:19:18]

I tried this with my local branch and I noticed no slow-down (I'm sure there is
one, but it certainly is not prohibitive).  It gets rid of any potential
segfaults from bad property sets in the layout block.

I realize now that all \set calls are, in fact, events and thus go through a
different function (set_property_from_event) that does this type checking.

Cheers,
MS

[Reinhold, 2011-07-07 11:02:59]

http://codereview.appspot.com/4654090/diff/1/lily/context.cc
File lily/context.cc (right):

http://codereview.appspot.com/4654090/diff/1/lily/context.cc#newcode496
lily/context.cc:496: properties_dict ()->set (sym, val);
else if (do_internal_type_checking_global) {
  assert(...);
}

Otherwise do_internaltype_checking_global won't give any indication that
something was wrong!
Also, without do_internal_type_checking_global set, I think we should give some
warning that the value was not assigned. Otherwise LilyPond silently discards a
user setting and the user has no idea why it doesn't have any effect (if he e.g.
uses a string instead of a symbol).

Also,

[Neil Puttock, 2011-07-07 16:37:47]

On 7 July 2011 12:02,  <reinhold.kainhofer@gmail.com> wrote:
>
> http://codereview.appspot.com/4654090/diff/1/lily/context.cc
> File lily/context.cc (right):
>
> http://codereview.appspot.com/4654090/diff/1/lily/context.cc#newcode496
> lily/context.cc:496: properties_dict ()->set (sym, val);
> else if (do_internal_type_checking_global) {
>  assert(...);
> }
>
> Otherwise do_internaltype_checking_global won't give any indication that
> something was wrong!

I agree, though the assert should come first otherwise
type_check_assignment () gets called twice.

Cheers,
Neil

[hanwenn, 2011-07-07 16:38:39]

On Wed, Jul 6, 2011 at 4:19 AM,  <mtsolo@gmail.com> wrote:
> Reviewers: ,
>
> Message:
> I tried this with my local branch and I noticed no slow-down (I'm sure
> there is one, but it certainly is not prohibitive).  It gets rid of any
> potential segfaults from bad property sets in the layout block.

Can you do a real measurement ? Have the code print out timestamps at
start & end of translation, and run a 10 times with and without.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[mike_apollinemike.com, 2011-07-08 08:48:59]

On Jul 7, 2011, at 6:38 PM, Han-Wen Nienhuys wrote:

> On Wed, Jul 6, 2011 at 4:19 AM,  <mtsolo@gmail.com> wrote:
>> Reviewers: ,
>> 
>> Message:
>> I tried this with my local branch and I noticed no slow-down (I'm sure
>> there is one, but it certainly is not prohibitive).  It gets rid of any
>> potential segfaults from bad property sets in the layout block.
> 
> Can you do a real measurement ? Have the code print out timestamps at
> start & end of translation, and run a 10 times with and without.
> 

Note that there is an outlier in the no context property checking.  I've shown
means with and without the largest/smallest dropped.

Time of translation (just before the call to lilypond-main and start of
interpretation and just before the message "Interpreting..."), 10 runs with
context property checking (in microseconds)
[261539, 261617, 261597, 261030, 265018, 260641, 282217, 262489, 262616, 265436]
mean: 264420.0
mean w/ outliers dropped: 262667.75

Time of translation, 10 runs with no context property checking (in microseconds)
[258837, 259777, 261231, 261915, 261306, 259702, 260661, 260903, 262827, 317991]
mean: 266515.0
mean w/ outliers dropped: 261040.25

Cheers,
MS

[c_sorensen, 2011-07-08 12:56:40]

On 7/8/11 2:48 AM, "mike@apollinemike.com" <mike@apollinemike.com> wrote:

> On Jul 7, 2011, at 6:38 PM, Han-Wen Nienhuys wrote:
> 
>> On Wed, Jul 6, 2011 at 4:19 AM,  <mtsolo@gmail.com> wrote:
>>> Reviewers: ,
>>> 
>>> Message:
>>> I tried this with my local branch and I noticed no slow-down (I'm sure
>>> there is one, but it certainly is not prohibitive).  It gets rid of any
>>> potential segfaults from bad property sets in the layout block.
>> 
>> Can you do a real measurement ? Have the code print out timestamps at
>> start & end of translation, and run a 10 times with and without.
>> 
> 
> Note that there is an outlier in the no context property checking.  I've shown
> means with and without the largest/smallest dropped.
> 
> Time of translation (just before the call to lilypond-main and start of
> interpretation and just before the message "Interpreting..."), 10 runs with
> context property checking (in microseconds)
> [261539, 261617, 261597, 261030, 265018, 260641, 282217, 262489, 262616,
> 265436]
> mean: 264420.0
> mean w/ outliers dropped: 262667.75
> 
> Time of translation, 10 runs with no context property checking (in
> microseconds)
> [258837, 259777, 261231, 261915, 261306, 259702, 260661, 260903, 262827,
> 317991]
> mean: 266515.0
> mean w/ outliers dropped: 261040.25

According to my statistics, there are no outliers (i.e., no points that are
more than 3 standard deviations away from the mean.

Whether one eliminates the "outliers", or whether one does not, the
conclusion is the same.  There is no statistically significant difference in
translation time between the two versions. (The version with context
checking has a slightly smaller mean in this data set, but the difference is
not even close to being statistically significant  -- 1/3 of the smaller
standard deviation, 1/9 of the larger).

The statistical conclusion has to be "no slow-down".

Thanks,

Carl

[MikeSol, 2011-07-10 08:31:44]

On 2011/07/08 12:56:40, c_sorensen_byu.edu wrote:
> 
> The statistical conclusion has to be "no slow-down".
> 
> Thanks,
> 
> Carl
> 

New patch set uploaded that keeps do_internal_type_checking_global.

Cheers,
MS

[Neil Puttock, 2011-07-10 21:04:29]

http://codereview.appspot.com/4654090/diff/8001/lily/context.cc
File lily/context.cc (right):

http://codereview.appspot.com/4654090/diff/8001/lily/context.cc#newcode496
lily/context.cc:496: assert (ok);
This doesn't tell you anything about the assertion failure (unlike the existing
code).

[pkx166h, 2011-07-10 21:19:13]

Passes make and reg tests - I'd open a tracker if I understood what this was
about :)

[mike_apollinemike.com, 2011-07-10 21:32:43]

On Jul 10, 2011, at 11:04 PM, n.puttock@gmail.com wrote:

> 
> http://codereview.appspot.com/4654090/diff/8001/lily/context.cc
> File lily/context.cc (right):
> 
> http://codereview.appspot.com/4654090/diff/8001/lily/context.cc#newcode496
> lily/context.cc:496: assert (ok);
> This doesn't tell you anything about the assertion failure (unlike the
> existing code).
> 
> http://codereview.appspot.com/4654090/
> 

New patchset uploaded - is that the sorta thing you're talking about?  I can
change the name of the variable to something else if you think that'd work
better.

Cheers,
MS

[reinhold_kainhofer.com, 2011-07-10 21:45:41]

Am Sonntag, 10. Juli 2011, 23:19:13 schrieb pkx166h@gmail.com:
> Passes make and reg tests - I'd open a tracker if I understood what this
> was about :)

that's actually a fix for bug 1734 (since it prevents the user from setting the 
properties in a layout block to a wrong type, which would then lead to the 
crash of bug 1734).

Cheers,
Reinhold
-- 
------------------------------------------------------------------
Reinhold Kainhofer, reinhold@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial & Actuarial Math., Vienna Univ. of Technology, Austria
 * http://www.fam.tuwien.ac.at/, DVR: 0005886
 * LilyPond, Music typesetting, http://www.lilypond.org

[Neil Puttock, 2011-07-10 21:56:11]

On 2011/07/10 21:32:43, mike_apollinemike.com wrote:

> New patchset uploaded - is that the sorta thing you're talking about?  I can
> change the name of the variable to something else if you think that'd work
> better.

Hah, that's a bit too much.  :)

A more descriptive name than `ok' will do (e.g., type_check_ok)

Cheers,
Neil

[MikeSol, 2011-07-11 11:40:08]

On 2011/07/10 21:56:11, Neil Puttock wrote:
> Hah, that's a bit too much.  :)
> 
> A more descriptive name than `ok' will do (e.g., type_check_ok)
> 
> Cheers,
> Neil

type_check_ok ok - added in a new patchset.

Cheers,
MS

[Neil Puttock, 2011-07-11 19:17:39]

http://codereview.appspot.com/4654090/diff/16002/lily/context.cc
File lily/context.cc (right):

http://codereview.appspot.com/4654090/diff/16002/lily/context.cc#newcode496
lily/context.cc:496: bool type_check_ok;
?

assert (type_check_ok);

[MikeSol, 2011-07-12 07:18:57]

http://codereview.appspot.com/4654090/diff/16002/lily/context.cc
File lily/context.cc (right):

http://codereview.appspot.com/4654090/diff/16002/lily/context.cc#newcode496
lily/context.cc:496: bool type_check_ok;
On 2011/07/11 19:17:39, Neil Puttock wrote:
> ?
> 
> assert (type_check_ok);

Wow...I was really tired when I did that.  Fixed!

[Neil Puttock, 2011-07-12 21:23:49]

LGTM.

[Carl, 2011-07-14 14:56:00]

LGTM.

With Neil's approval I think we can go ahead and push.

[MikeSol, 2011-07-15 16:16:59]

On 2011/07/14 14:56:00, Carl wrote:
> LGTM.
> 
> With Neil's approval I think we can go ahead and push.

Thanks.
Pushed as a811a3c91c05f33474c1d447bedaa1e089522532.

Cheers,
MS