Re: GUILE_MAX_HEAP_SIZE

[Ludovic Courtès]

Han-Wen Nienhuys <address@hidden> writes:

> Ludovic Courtès escreveu:

>>   * 51ef99f7fa9fb766fbb48619fc5863ab9914591d
>>     Fix memory corruption issue with hell[] array: realloc/calloc need to
>>     factor in sizeof(scm_t_bits)
>> 
>>     -  hell = scm_malloc (hell_size);
>>     +  hell = scm_calloc (hell_size * sizeof(scm_t_bits));
>> 
>>     Good catch, but it should read:
>> 
>>        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?

>
>>   * b61b5d0ebea924ee4b3930b2cba72e282d4751c7
>>     Do not include private-gc.h in srfi-60.
>> 
>>     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.

>
>>   * 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?

>>   * d09752ffd17688b33a1e828cf4c11f66b86c3c3c
>>     Introduce scm_i_marking to detect when GC mark bits are touched
>>     outside of marking stage.
>> 
>>     `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.

> By rolling back changes, you have just robbed me of the opportunity to see
> what it was I did wrong.

I'd have preferred it if your changes had remained in your branch while
we discuss them.  We'd have been able to take time to understand them,
and I wouldn't have had to rush because the thing is already committed.

> Can you remind me again of the sha1 of the stable 
> branch?  AFAICR I only pushed trivial fixes there.

Right, my mistake.  The stable branch is `branch_release-1-8'.

> 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).

> It's not that I see many reviews of your commits on the list passing by. 

Please, see the archive of `guile-devel'.  I rarely commit without
posting here first, and it's proved to be a good tool to improve
quality.

Thanks,
Ludo'.