Re: Part_combine_iterator::derived_mark: don't abort marking prematurely. (issue 6057044)

[Han-Wen Nienhuys]

On Wed, Apr 18, 2012 at 4:56 AM,  d...@gnu.org wrote:
 Short of any protests, I think I'll be going with

 void
 Part_combine_iterator::derived_mark () const
 {
  if (first_iter_)
    scm_gc_mark (first_iter_-self_scm ());
  if (second_iter_)
    scm_gc_mark (second_iter_-self_scm ());
  if (unisono_event_)
    scm_gc_mark (unisono_event_-self_scm ());
  if (mmrest_event_)
    scm_gc_mark (mmrest_event_-self_scm ());
  if (solo_one_event_)
    scm_gc_mark (solo_one_event_-self_scm ());
  if (solo_two_event_)
    scm_gc_mark (solo_two_event_-self_scm ());
 }

 All the rest is too smart for its own good.

FYI,  my experience is that writing this type of code involves cut 
paste, and it is easy to make errors like

 if (some_new_event_)
   mark(the_event_i_copied_it_from_)

I agree that 4 is borderline small enough not to use a loop.

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

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

Part_combine_iterator::derived_mark: don't abort marking prematurely. (issue 6057044)

[graham]

LGTM and can be pushed right now.

But for my general ignorance: why not just hard-code 4?  I suppose that
using the sizeof() trick makes it a bit safer in case somebody adds
something to ptrs, but is there any other reason?

http://codereview.appspot.com/6057044/

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

Re: Part_combine_iterator::derived_mark: don't abort marking prematurely. (issue 6057044)

[dak]

Reviewers: Graham Percival,

Message:
On 2012/04/17 20:21:14, Graham Percival wrote:

LGTM and can be pushed right now.



But for my general ignorance: why not just hard-code 4?  I suppose

that using

the sizeof() trick makes it a bit safer in case somebody adds

something to ptrs,

but is there any other reason?


No.  It is just a common C idiom.  Personally, I think it would make
more sense to just unfold the loop.  Takes fewer lines and is clearer.

I just kept with the original style.

For an array-based style, an automatic array makes little sense over
explicit code.  One could instead use a static array with
pointers-to-member (basically offsets), and then dereference them. In
that case, a 0 entry would indeed work as an ending mark.

But for 4 entries?

Description:
Part_combine_iterator::derived_mark: don't abort marking prematurely.

Please review this at http://codereview.appspot.com/6057044/

Affected files:
  M lily/part-combine-iterator.cc


Index: lily/part-combine-iterator.cc
diff --git a/lily/part-combine-iterator.cc b/lily/part-combine-iterator.cc
index  
eb1cca43e57092ed886f3166c6ae739d97c03970..5378aff4ecf405614aab7a9a978cca166193b90c  
100644

--- a/lily/part-combine-iterator.cc
+++ b/lily/part-combine-iterator.cc
@@ -165,10 +165,9 @@ Part_combine_iterator::derived_mark () const
 unisono_event_,
 mmrest_event_,
 solo_two_event_,
-solo_one_event_,
-0
+solo_one_event_
   };
-  for (int i = 0; ptrs[i]; i++)
+  for (vsize i = 0; i  sizeof (ptrs)/sizeof (*ptrs); i++)
 if (ptrs[i])
   scm_gc_mark (ptrs[i]-self_scm ());
 }



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

Re: Part_combine_iterator::derived_mark: don't abort marking prematurely. (issue 6057044)

[dak]

On 2012/04/18 04:39:20, dak wrote:

One could instead use a static array with pointers-to-member

(basically

offsets), and then dereference them. In that case, a 0 entry would

indeed work

as an ending mark.



But for 4 entries?


To illustrate:

  static Stream_event * Part_combine_iterator::* ptrs[]
  =
  {
Part_combine_iterator::unisono_event_,
Part_combine_iterator::mmrest_event_,
Part_combine_iterator::solo_two_event_,
Part_combine_iterator::solo_one_event_,
0
  };
  for (int i = 0; ptrs[i]; i++)
if (ptrs[i])
  scm_gc_mark ((this-*ptrs[i])-self_scm ());


http://codereview.appspot.com/6057044/

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

Re: Part_combine_iterator::derived_mark: don't abort marking prematurely. (issue 6057044)

[dak]

On 2012/04/18 05:49:01, dak wrote:

On 2012/04/18 04:39:20, dak wrote:
 One could instead use a static array with pointers-to-member

(basically

 offsets), and then dereference them. In that case, a 0 entry would

indeed work

 as an ending mark.

 But for 4 entries?



To illustrate:



   static Stream_event * Part_combine_iterator::* ptrs[]
   =
   {
 Part_combine_iterator::unisono_event_,
 Part_combine_iterator::mmrest_event_,
 Part_combine_iterator::solo_two_event_,
 Part_combine_iterator::solo_one_event_,
 0
   };
   for (int i = 0; ptrs[i]; i++)
 if (ptrs[i])

Uh, if (this-*ptrs[i]) of course

   scm_gc_mark ((this-*ptrs[i])-self_scm ());




http://codereview.appspot.com/6057044/

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