[Webkit-unassigned] [Bug 27551] Make it possible to build JavaScriptCore as shared library without symbol lists

[bugzilla-daemon at webkit.org]

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





--- Comment #113 from Kevin Ollivier <kevino at theolliviers.com>  2011-03-13 20:23:15 PST ---
(In reply to comment #112)
> (From update of attachment 84999 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84999&action=review
> 
> Concept seems fine and execution seems OK.
> 
> The naming is a little inconsistent. We have JS_EXPORTDATA but JS_EXPORT_PRIVATE, so two different styles in how to deal with multiple words.

This is temporary, and it will all end up being JS_EXPORT_PRIVATE in the end. I am leaving this temporarily to not break any port we haven't yet moved to using the export macros.

> > Tools/WebKitAPITest/HostWindow.cpp:27
> > +#include "config.h"
> > +
> >  #include "HostWindow.h"
> 
> Normally our style is to not have a blank line between the include of config.h and the include of the file's own header. We could have a blank line if the first header is not the file’s own.

Okay, will fix before landing.

> >> Tools/WebKitAPITest/config.h:25
> >> +#ifndef WebKitAPITests_config_h
> > 
> > #ifndef header guard has wrong style, please use: WTF_config_h  [build/header_guard] [5]
> 
> False error from the style tool. We should file another bug asking for it to be fixed.

Okay, I'll post a bug about this.

> > Tools/WebKitAPITest/config.h:31
> > +#if USE(EXPORT_MACROS)
> 
> Should have a blank line after this since we have a blank line before the #else and the #endif.

Okay, will fix before landing.

> > Tools/WebKitAPITest/config.h:41
> > +#else /* !USE(EXPORT_MACROS) */
> 
> I’m concerned that the boilerplate section of the config files are so long! Seems like plenty of room for error. Is there any way to avoid having so much boilerplate?

Well, first, the !USE(EXPORT_MACROS) case (and the USE check itself) will disappear once all ports have moved to using it, ideally in a few weeks or so. That being said, what I'd eventually like to do is simply to have some headers like WTFDefines.h, JSDefines.h, etc. that defines the various export macros they use and are simply included by the various config.h files, or maybe even directly by the headers that need them. There is a lot of duplicate code defining the various macros in each config.h, and it'd be nice to just include a header instead of re-defining them over and over.

This and removing JS_EXPORTDATA were things I started trying to handle on the first attempt to address this, but it led to having to figure out the issues with all the build systems at once, and this caused the patch to become unwieldily to maintain as the fixes piled up. So I'd like to try all these things, but once we've got the export macros working in basic form everywhere first.

> >> Source/JavaScriptCore/wtf/Assertions.h:151
> >> +WTF_EXPORT_PRIVATE void WTFLog(WTFLogChannel* channel, const char* format, ...) WTF_ATTRIBUTE_PRINTF(2, 3);
> > 
> > The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> I agree with the style queue about this.
> 
> >> Source/JavaScriptCore/wtf/Assertions.h:152
> >> +WTF_EXPORT_PRIVATE void WTFLogVerbose(const char* file, int line, const char* function, WTFLogChannel* channel, const char* format, ...) WTF_ATTRIBUTE_PRINTF(5, 6);
> > 
> > The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> And here too.

Okay, then I'll change them before landing. Many thanks for reviewing this! :)

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