[Webkit-unassigned] [Bug 27980] Give an ability to WebKit to free statically allocated pointers before quit

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27980


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #41886|review?                     |review-
               Flag|                            |




--- Comment #41 from Darin Adler <darin at apple.com>  2009-10-27 11:37:45 PDT ---
(From update of attachment 41886)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.