[webkit-reviews] review granted: [Bug 211729] Introduce WTF::Config and put Signal.cpp's init-once globals in it. : [Attachment 399031] proposed patch.

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Mark Lam
<mark.lam at apple.com>'s request for review:
Bug 211729: Introduce WTF::Config and put Signal.cpp's init-once globals in it.
https://bugs.webkit.org/show_bug.cgi?id=211729

Attachment 399031: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=399031&action=review




--- Comment #10 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 399031
  --> https://bugs.webkit.org/attachment.cgi?id=399031
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=399031&action=review

r=me with comment.

>>>>> Source/WTF/ChangeLog:3
>>>>> +        Introduce WTF::Config and put Signal.cpp's init-once globals in
it.
>>>> 
>>>> WebKit coding style says we use words, not abbreviations. Why "Config"?
>>> 
>>> Config is commonly used all over our code and is well recognized to mean
configuration e.g. config.h is not named configuration.h.  The reason I use
Config is because it contains all the effectively "constant" global values that
configures the behavior of our system.	The are initialized and then expected
to remain unchanged by our code (hence, effectively "constant").  The current
instances of these Configs are g_gigacageConfig, g_wtfConfig, and g_jscConfig. 
I'm open to a better name if you have one that is concise and short.
>> 
>> Configuration is not long and it’s a normal word.
>> 
>> The header "config.h" was not named by the WebKit project; we chose it for
compatibility with autoconf.
>> 
>> I don’t think the names g_gigacageConfig, g_wtfConfig, and g_jscConfig are
consistent with WebKit coding style.
> 
> 1. I think of Config as a term of art.  It's so commonly used and understood
in software.
>    We don't special out VirtualMachine instead of VM, DocumentObjectModel
instead of DOM, etc.
> 
> 2. I did not quote 3 examples to convince you of the precedence, merely to
point out that it would be longer to type things like this:
> 
>     g_gigacageConfiguration.ensureGigacageHasBeenCalled
>     g_gigacageConfiguration.shouldBeEnabledHasBeenCalled
>     g_jscConfiguration.startOfFixedWritableMemoryPool
> 
>    It adds more typing and reading with 0 benefit in code comprehension.  If
anything, it slightly adds a bit of mental friction to read code because one
would have to process more text.
> 
> 3. Precedence.
> 
>    % cd Source
>    % grep -rn Config * | grep -v Configuration | grep -v Configurable | grep
-v ChangeLog | grep -v props | grep -vi make | grep -vi xcode | grep -v '.pyc'
| grep -v ThirdParty | grep -v 'Scripts/' | wc
>	1024	5432  119346
> 
>    Minus my uses (approximately):
>    % grep -rn Config * | grep -v Configuration | grep -v Configurable | grep
-v ChangeLog | grep -v props | grep -vi make | grep -vi xcode | grep -v '.pyc'
| grep -v ThirdParty | grep -v 'Scripts/' | grep -v g_wtfConfig | grep -v
g_gigacageConfig | grep -v g_jscConfig | grep -v WTF::Config | grep -v
JSC::Config | grep -v Gigacage::Config | wc
>	  891	 4827  104559
> 
> But if you strongly object to my use of it, I'll change mine to
Configuration.

For what it's worth, Config is pretty widely used but it Configuration is more
common. I don't really care either way though.

$ ag -i "configuration" Source --ignore Source/ThirdParty --ignore
"*ChangeLog*" --stats-only
7861 matches
604 files contained matches

The [^xc] is to skip xcconfig files. If you count them then I guess config is
more common.
$ ag -i "[^xc]config[^u|^\.]" Source --ignore Source/ThirdParty --ignore
"*ChangeLog*" --stats-only
1900 matches
363 files contained matches

> Source/WTF/wtf/threads/Signals.cpp:326
> -	   dataLogLn("We somehow got called for an unknown signal ", sig, ",
halp.");
> +	   dataLogLn("We somehow got called for an unknown signal ", sig, ",
help.");

This spelling was intentional :P

> Source/WTF/wtf/threads/Signals.h:93
> +    SignalHandlerMemory* alloc(Signal);

This is bizarre. I think this should be: add(Signal, SignalHandler&&);