[webkit-reviews] review denied: [Bug 21810] Remove use of static C++ objects that are destroyed at exit time (destructors) : [Attachment 25102] This patch uses a macro

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Greg Bolsinga <bolsinga at apple.com>'s
request for review:
Bug 21810: Remove use of static C++ objects that are destroyed at exit time
(destructors)
https://bugs.webkit.org/show_bug.cgi?id=21810

Attachment 25102: This patch uses a macro
https://bugs.webkit.org/attachment.cgi?id=25102&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   * wtf/Platform.h: Add DEFINE_STATIC_LOCAL macro

This file is not the right place for the macro. I know it's tempting because
it's a header that's "included everywhere", but as you discovered while working
on the patch, it's not included everywhere. And the header is supposed to
contain only platform-configuration macros, not general purpose ones like this.


Lets put this in its own separate header file. As we discussed on IRC, I think
it could be <wtf/StdLibExtras.h> since this is closely related to exit() and
atexit() behavior. I think it might be OK to stick this into "config.h" files
so we don't have to include it everywhere, or add the includes everywhere we
need it.

> +typedef HashMap<int, PropertyLonghand> ShortHandMap;

Since "shorthand" is one word, it should be ShorthandMap, not ShortHandMap.
Also, to match the other names that use this.

> -	       static const String rectParen("rect(");
> +	       DEFINE_STATIC_LOCAL(String, rectParen, ("rect("));

You're removing the "const" here and in many other places too.

    DEFINE_STATIC_LOCAL(const String, rectParen, ("rect("));

The above would work and I think be slightly better because it preserves the
const. These globals can be const or non-const. We could even make some more
const that weren't previously const.

> -    static Cursor& c = leakNamedCursor("crossHairCursor", 11, 11);
> +    static Cursor* cPtr = leakNamedCursor("crossHairCursor", 11, 11);
> +    Cursor& c = *cPtr;

These aren't using the macro. They should.

> -    static HashMap<String, int>& stringIdentifierToGUIDMap = *new
HashMap<String, int>;
> +    typedef HashMap<String, int> IDGuidMap;
> +    DEFINE_STATIC_LOCAL(IDGuidMap, stringIdentifierToGUIDMap, ());

Are you adding these typedefs for clarity, or does the macro malfunction if you
don't use them? I would have tried something like this:

    DEFINE_STATIC_LOCAL(HashMap<String, int>, stringIdentifierToGUIDMap, ());

I think when you land this it should be in small pieces. The first patch could
just add the macro and include the JavaScriptCore changes. Then you could do
the WebCore change and the WebKit change each as a separate patch or a number
of them.

Then you could finish up with the tools list change as a separate patch.

This would be especially good if some of these cause problems. Someone could
figure out which patch broke things if the patches are smaller pieces.

review- because of the Platform.h issue