[webkit-reviews] review denied: [Bug 4084] Conservative Garbage Collector - Build on Windows : [Attachment 3031] Patch to build collector.cpp - works

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Justin Haygood
<justin at xiondigital.net>'s request for review:
Bug 4084: Conservative Garbage Collector - Build on Windows
http://bugzilla.opendarwin.org/show_bug.cgi?id=4084

Attachment 3031: Patch to build collector.cpp - works
http://bugzilla.opendarwin.org/attachment.cgi?id=3031&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good. Some comments that we should resolve before landing this:

A) The THREADING define should have a more-specific name so it's easier to
understand what it means. I suggest KJS_MULTIPLE_THREAD_SUPPORT, but perhaps we
can come up with something even better.

B) As with the other files using the non-standard MAX, I suggest switching to
the C++ std::max rather than defining MAX for Windows.

C) The comments:

    +// Necessary Includes

and

    +// Necessary Defines

aren't helpful and should be omitted.

D) Although I don't think it's mandatory to resolve this for this patch, the
idea of having the switch for multiple thread support only inside this file
isn't really sufficient. The interpreter lock, implemented in internal.cpp, is
also only helpful if you're supporting multiple threads, and the
multiple-thread switch should turn that off too.

E) The WIN32 code includes a macro, is_writable. Normally, we use all capitals
for our macros, so it should be IS_WRITABLE. In this case, I think we shouldn't
use a macro -- we can just put that code in line. But further, I'm not sure
what the "is writable" check accomplishes. If the stack isn't writable, then we
have something very strange going on; I don't think for our purposes we need
that code at all. We just need the RegionSize value returned from VirtualQuery.


F) In general, the code imported for the stack base determination needs to be
updated for code style. The function names shoud not start with GC, and they
should not use underscores between words. The code should also be formatted
based on our coding style.

G) Since we're not using the base parameter of the GC_get_writable_length
function we can remove that part of the function, and in general I think we're
doing something simple enough that we could put all the logic inside a single
"get stack base" function without sacrificing clarity -- in fact I think it
would make things more clear to do that.

H) Since there's only a small amount of code here, I don't think the typedefs
for "word" and "ptr_t" add to clarity. We should probably just use unsigned
long and char * directly.

I) The new functions and global variables defined here and used only inside
this source file should have static in front of them.

J) The "PLTSCHEME" comment is wrong for our code base. It's referring to other
use of the GC_page_size variable in the original code that doesn't apply in
KJS.

K) I don't think caching the page size is important. A call to GetSystemInfo
each time seems fine since we're doing a VirtualQuery call each time too.

L) Although I'm not a Windows expert I think there's a simpler way to do this
that will work. Here's my attempt; maybe you can test it to see if it works. If
it does, you don't have to worry about (E), (F), (G), (H), (I), (J), and (K)
above!

    static void *stackBase()
    {
	int dummy; // use to find top of stack
	MEMORY_BASIC_INFORMATION memoryInfo;
	VirtualQuery(&dummy, &memoryInfo, sizeof(memoryInfo));
	return static_cast<char *>(memoryInfo.BaseAddress) +
memoryInfo.RegionSize;
    }

If my version works, I think it's better than the original one; in fact I think
the old code would give an incorrect value for the stack base in cases where
the stack was multiple pages because it adds the size of the region to the
current stack pointer, but the current stack pointer may be in the middle of
the region, not at ths start.

There's no real way to recover if VirtualQuery returns an error, and also no
reason to expect that it could fail when called on the stack. That's why I
didn't try to write any error-handling code.

M) Here as in other patches there is a mix of #if WIN32 with #ifndef WIN32.
Either it sould be #if WIN32 and #if !WIN32, or #ifdef WIN32 and #ifndef WIN32.


N) I think the setjmp(registers) code is needed on WIN32 too, for the same
reason that it's needed on Mac OS X so it should not be inside the ifdef.