[webkit-reviews] review denied: [Bug 27980] Give an ability to WebKit to free statically allocated pointers before quit : [Attachment 41886] Proposed Partial Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Carol Szabo <carol.szabo at nokia.com>'s
request for review:
Bug 27980: Give an ability to WebKit to free statically allocated pointers
before quit
https://bugs.webkit.org/show_bug.cgi?id=27980

Attachment 41886: Proposed Partial Patch
https://bugs.webkit.org/attachment.cgi?id=41886&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
It seems fine to use DEFINE_STATIC_LOCAL in one more place. And since that's
all the patch does, we don't have to have any deep debate about it.

It also seems OK to allow a version of DEFINE_STATIC_LOCAL defined elsewhere to
override the version in StdLibExtras.h, but I don't see any reason the two need
to be done in a single patch.

> +#ifndef DEFINE_STATIC_LOCAL
>  #if COMPILER(GCC) && defined(__APPLE_CC__) && __GNUC__ == 4 &&
__GNUC_MINOR__ == 0 && __GNUC_PATCHLEVEL__ == 1
>  #define DEFINE_STATIC_LOCAL(type, name, arguments) \
>      static type* name##Ptr = new type arguments; \
> @@ -40,7 +41,7 @@
>  #define DEFINE_STATIC_LOCAL(type, name, arguments) \
>      static type& name = *new type arguments
>  #endif
> -
> +#endif
>  // OBJECT_OFFSETOF: Like the C++ offsetof macro, but you can use it with
classes.
>  // The magic number 0x4000 is insignificant. We use it to avoid using NULL,
since
>  // NULL can cause compiler problems, especially in cases of multiple
inheritance.

You should move the #endif here so that there's still a blank line before
OBJECT_OFFSETOF.

> +	   * platform/ThreadGlobalData.cpp:
> +	   (WebCore::threadGlobalData):
> +	   Allocated threadGlobalData statically, not on heap such that it
> +	   will be destroyed when the library is unloaded.

I do not think the comment here is clear. My comment would be something more
like, "Use DEFINE_STATIC_LOCAL macro instead of doing a one-offer version here.
Helpful because we are using DEFINE_STATIC_LOCAL to experiment with unloading
the library and deallocating memory".

I think these patches are OK, but the change log should talk about what they do
rather than saying these give WebKit ability to free memory when unloading.
That's a big project that this patch is only a tiny part of.

review- because of the minor problem with #endif and because the change log
could be a lot clearer, but the patch seems fine otherwise