Bug 183545 – PerProcess<> should be safe by default

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 335519 [details]
maybe this will work

[Filip Pizlo]

Created attachment 335520 [details]
the patch

[EWS Watchlist]

Attachment 335520 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/PerProcess.h:64:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/PerProcess.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 335520 [details]
the patch

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

r=me with a micro nit.

> Source/bmalloc/bmalloc/PerProcess.cpp:49
> +static unsigned stringHash(const char* string)
> +{
> +    unsigned result = 5381;
> +    while (char c = *string++)
> +        result = result * 33 + c;
> +    return result;
> +}

Nits: you can make this `constexpr` function, move it to a header, and calculate hash value at compile time.

[Filip Pizlo]

(In reply to Yusuke Suzuki from comment #4)
> Comment on attachment 335520 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335520&action=review
> 
> r=me with a micro nit.
> 
> > Source/bmalloc/bmalloc/PerProcess.cpp:49
> > +static unsigned stringHash(const char* string)
> > +{
> > +    unsigned result = 5381;
> > +    while (char c = *string++)
> > +        result = result * 33 + c;
> > +    return result;
> > +}
> 
> Nits: you can make this `constexpr` function, move it to a header, and
> calculate hash value at compile time.

Really, the compiler will execute a constexpr loop?

[Filip Pizlo]

Created attachment 335540 [details]
patch for landing

With Yusuke's suggested change.

[EWS Watchlist]

Attachment 335540 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/PerProcess.h:72:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/PerProcess.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

(In reply to Filip Pizlo from comment #5)
> > Nits: you can make this `constexpr` function, move it to a header, and
> > calculate hash value at compile time.
> 
> Really, the compiler will execute a constexpr loop?

Yeah, C++14 relaxed constexpr is super powerful. You can check our wtf/text/StringHasher.h's computeLiteralHash for example.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/229516/webkit

[Radar WebKit Bug Importer]

<rdar://problem/38397867>

[Joseph Pecoraro]

*** Bug 182496 has been marked as a duplicate of this bug. ***