Code review - Issue 6057044: Part_combine_iterator::derived_mark: don't abort marking prematurely.https://codereview.appspot.com/2012-04-22T11:32:05+00:00rietveld
Message from unknown
2012-04-17T09:04:36+00:00dakurn:md5:60f24c8fd00f8d653168c3d27cfb1b2a
Message from graham@percival-music.ca
2012-04-17T20:21:14+00:00Graham Percivalurn:md5:ccfc5ff7cf7d89fe3dc483b374b6115a
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?
Message from dak@gnu.org
2012-04-18T04:39:20+00:00dakurn:md5:8f18f8b0e1cd9109cb8afa0660f5207c
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?
Message from dak@gnu.org
2012-04-18T05:49:01+00:00dakurn:md5:b332e96141d2539f3b5ce97d9af3d3c8
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 ());
Message from dak@gnu.org
2012-04-18T05:51:23+00:00dakurn:md5:6df1d36fad8db537ff685d004a600fa2
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 ());
Message from dak@gnu.org
2012-04-18T07:56:39+00:00dakurn:md5:ea72958b98839ce97a7527f448657427
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.
Message from unknown
2012-04-18T08:03:09+00:00dakurn:md5:95f0f03acc1f050b2b68a44a222d76c2
Message from hanwenn@gmail.com
2012-04-22T11:32:05+00:00hanwennurn:md5:80970e48d45ee12ade8f6a2eaecb79ef
On Wed, Apr 18, 2012 at 4:56 AM, <dak@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 - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen