Bug 171861 - Force StaticStringImpl constructor to use the constexpr versions of StringImplShape constructors.
Summary: Force StaticStringImpl constructor to use the constexpr versions of StringImp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 171800
  Show dependency tree / graph
 
Reported: 2017-05-09 08:50 PDT by Mark Lam
Modified: 2017-05-09 12:05 PDT (History)
9 users (show)

See Also:

Attachments
proposed patch. (3.33 KB, patch)
2017-05-09 09:12 PDT, Mark Lam 		ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing. (3.52 KB, patch)
2017-05-09 09:25 PDT, Mark Lam 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-05-09 08:50:47 PDT 
This is strictly necessary for correctness of the StaticStringImpl implementation.
Comment 1 Mark Lam 2017-05-09 09:12:04 PDT 
Created attachment 309503 [details]
proposed patch.
Comment 2 Yusuke Suzuki 2017-05-09 09:15:21 PDT 
Comment on attachment 309503 [details]
proposed patch.

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

r=me with nits.

> Source/WTF/wtf/text/StringImpl.h:150
> +    constexpr StringImplShape(unsigned refCount, unsigned length, const char (&characters)[charactersCount], unsigned hashAndFlags, int)

I like to use enum for these taggings like, CreateSymbolTag in StringImpl.h.

> Source/WTF/wtf/text/StringImpl.h:158
> +    constexpr StringImplShape(unsigned refCount, unsigned length, const char16_t (&characters)[charactersCount], unsigned hashAndFlags, int)

Ditto.
Comment 3 Mark Lam 2017-05-09 09:16:31 PDT 
(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 309503 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309503&action=review
> 
> r=me with nits.
> 
> > Source/WTF/wtf/text/StringImpl.h:150
> > +    constexpr StringImplShape(unsigned refCount, unsigned length, const char (&characters)[charactersCount], unsigned hashAndFlags, int)
> 
> I like to use enum for these taggings like, CreateSymbolTag in StringImpl.h.

Good idea.  Will do.
Comment 4 Mark Lam 2017-05-09 09:25:12 PDT 
Created attachment 309505 [details]
patch for landing.
Comment 5 Mark Lam 2017-05-09 10:12:09 PDT 
Thanks for the review.  Landed in r216512: <http://trac.webkit.org/r216512>.
Comment 6 Saam Barati 2017-05-09 12:05:23 PDT 
Comment on attachment 309505 [details]
patch for landing.

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

> Source/WTF/wtf/text/StringImpl.h:160
> +    constexpr StringImplShape(unsigned refCount, unsigned length, const char16_t (&characters)[charactersCount], unsigned hashAndFlags, ConstructWithConstExprTag)

Style nit: I feel like we usually put these tags as the first argument, not the last.