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 - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
