Re: GUILE_MAX_HEAP_SIZE

[Han-Wen Nienhuys]

Ludovic Courtès escreveu:

>>>        hell = scm_calloc (hell_size * sizeof (*hell));
>>>
>>>     `sizeof (*hell)' is actually `sizeof (scm_t_bits *)', which is equal
>>>     to `sizeof (scm_t_bits)', but using `sizeof (*hell)' is clearer and
>>>     less error-prone.
>>>
>>>     Besides, is that code only used when the one changes the class of an
>>>     instance?  How did you trigger it?
>> valgrind. Fixed.
> 
> Sorry, I meant: which Scheme code leads to the execution of that code?

I have no idea. Some part of the test suite exercises it, but I think it is 
better not to leave such an obvious error there.


>>>     Hmm, well, we really need an `SCM_MIN ()' somewhere.  I'd rather
>>>     duplicate its definition than expand it as you did here, since that
>>>     makes the code rather unreadable.
>> I called private-gc.h private for a reason.   Please do not include it 
>> unless you are called libguile/gc*c; feel free to transplant SCM_MIN to
>> someplace else.
> 
> I agree on that, but I also think that expanding `SCM_MIN ()' in-place
> is not a good idea.

I agree with that, it's just that I prefer to let my responsibility end 
at the boundaries of the GC code.

>>>   * 569aa529d5379f3c942fa6eb01e8a1ad48ba9f77
>>>     Use word_2 to store mark bits for freeing structs and vtables in the
>>>     correct order.
>>>
>>>     Can you explain this?  Suppose we have struct S whose vtable is V;
>>>     V cannot be swept in the same GC cycle as S since it's still
>>>     referenced by S.  Thus, I don't understand the need for
>>>     special-casing here.
>> Freeing S requires a function stored in V.
> 
> Right, but my understanding is that V is still reachable (via S) when S
> becomes candidate for sweeping.  Is that right?

No, the freeing is all for unreachable objects. The problem is that unreachable
objects also may have an ordering: S needs to be freed before V, even if both 
are 
unreachable.

>>>     `ensure_marking ()' must be `static'.  The definition of
>>>     `scm_i_marking' clearly doesn't belong in a header.  Besides, all
>>>     this is unused, so what's the point?
>> I'm not sure where to put the code, perhaps in a ifdef DEBUG or something:
>> the point was to extend SCM_GC_SET_MARK with ensure_marking() to catch 
>> illegal 
>> use of the mark bits.
> 
> But it's actually unused (at least in this commit), so I'd just remove
> it.

Yes - it's not ideal.  I would vote for keeping the is_marking variable, and 
perhaps dropping the ensure_marking() function.  (My experience is that the 
#ifdef DEBUG sections are never exercised, because noone ever bothers to test
and use those secions.)


>> Also, if a core contributor apparently need some sort of review process to 
>> push 
>> code they feel comfortable with, can you please post a link to the process?
> 
> There's no such document, just an observation of what has been common
> practice since I follow Guile development (c. 2004).

FWIW, GUILE development seems from the outside very much stagnant, 
even if there are the occasional commits to the master branch.  Perhaps I have
various preconceptions because I also follow LilyPond development, which is 
more turbulent, with more mistakes going in at a higher pace, but also more
discussion and more bugfixing going in at a higher pace.

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