[webkit-reviews] review granted: [Bug 27551] Make it possible to build JavaScriptCore as shared library without symbol lists : [Attachment 84999] Retry - Step 1 w/ more config.h additions for projects in Tools

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kevin Ollivier
<kevino at theolliviers.com>'s request for review:
Bug 27551: Make it possible to build JavaScriptCore as shared library without
symbol lists
https://bugs.webkit.org/show_bug.cgi?id=27551

Attachment 84999: Retry - Step 1 w/ more config.h additions for projects in
Tools
https://bugs.webkit.org/attachment.cgi?id=84999&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.

> 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.

>> 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.

> 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.

> 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?

>> 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.