Han-Wen Nienhuys <[EMAIL PROTECTED]> 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'.
