Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(603)

Issue 4654090: Does type checking for all context property sets. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 8 months ago by MikeSol
Modified:
12 years, 8 months ago
Reviewers:
mike, carl.d.sorensen, Neil Puttock, c_sorensen, reinhold, Reinhold, pkx166h, hanwenn
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Does type checking for all context property sets.

Patch Set 1 #

Total comments: 1

Patch Set 2 : Keeps do_internal_type_checking_global #

Total comments: 1

Patch Set 3 : More info in the assert error. #

Patch Set 4 : Uses a shorter variable name stated in the affirmative. #

Total comments: 2

Patch Set 5 : Actually implements assert error. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -2 lines) Patch
M lily/context.cc View 1 2 3 4 1 chunk +5 lines, -2 lines 0 comments Download

Messages

Total messages: 18
MikeSol
I tried this with my local branch and I noticed no slow-down (I'm sure there ...
12 years, 8 months ago (2011-07-06 07:19:18 UTC) #1
Reinhold
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(...); ...
12 years, 8 months ago (2011-07-07 11:02:59 UTC) #2
Neil Puttock
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): > ...
12 years, 8 months ago (2011-07-07 16:37:47 UTC) #3
hanwenn
On Wed, Jul 6, 2011 at 4:19 AM, <mtsolo@gmail.com> wrote: > Reviewers: , > > ...
12 years, 8 months ago (2011-07-07 16:38:39 UTC) #4
mike_apollinemike.com
On Jul 7, 2011, at 6:38 PM, Han-Wen Nienhuys wrote: > On Wed, Jul 6, ...
12 years, 8 months ago (2011-07-08 08:48:59 UTC) #5
c_sorensen
On 7/8/11 2:48 AM, "mike@apollinemike.com" <mike@apollinemike.com> wrote: > On Jul 7, 2011, at 6:38 PM, ...
12 years, 8 months ago (2011-07-08 12:56:40 UTC) #6
MikeSol
On 2011/07/08 12:56:40, c_sorensen_byu.edu wrote: > > The statistical conclusion has to be "no slow-down". ...
12 years, 8 months ago (2011-07-10 08:31:44 UTC) #7
Neil Puttock
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 ...
12 years, 8 months ago (2011-07-10 21:04:29 UTC) #8
pkx166h
Passes make and reg tests - I'd open a tracker if I understood what this ...
12 years, 8 months ago (2011-07-10 21:19:13 UTC) #9
mike_apollinemike.com
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 ...
12 years, 8 months ago (2011-07-10 21:32:43 UTC) #10
reinhold_kainhofer.com
Am Sonntag, 10. Juli 2011, 23:19:13 schrieb pkx166h@gmail.com: > Passes make and reg tests - ...
12 years, 8 months ago (2011-07-10 21:45:41 UTC) #11
Neil Puttock
On 2011/07/10 21:32:43, mike_apollinemike.com wrote: > New patchset uploaded - is that the sorta thing ...
12 years, 8 months ago (2011-07-10 21:56:11 UTC) #12
MikeSol
On 2011/07/10 21:56:11, Neil Puttock wrote: > Hah, that's a bit too much. :) > ...
12 years, 8 months ago (2011-07-11 11:40:08 UTC) #13
Neil Puttock
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);
12 years, 8 months ago (2011-07-11 19:17:39 UTC) #14
MikeSol
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: > ...
12 years, 8 months ago (2011-07-12 07:18:57 UTC) #15
Neil Puttock
LGTM.
12 years, 8 months ago (2011-07-12 21:23:49 UTC) #16
Carl
LGTM. With Neil's approval I think we can go ahead and push.
12 years, 8 months ago (2011-07-14 14:56:00 UTC) #17
MikeSol
12 years, 8 months ago (2011-07-15 16:16:59 UTC) #18
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b