Bug 188146 - Use static global variables instead of static NeverDestroyed inside function if possible
Summary: Use static global variables instead of static NeverDestroyed inside function ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-29 06:56 PDT by Yusuke Suzuki
Modified: 2018-07-29 11:14 PDT (History)
7 users (show)

See Also:


Attachments
Patch (39.16 KB, patch)
2018-07-29 07:05 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-07-29 06:56:32 PDT
Use constexpr constructors more instead of NeverDestroyed
Comment 1 Yusuke Suzuki 2018-07-29 07:05:18 PDT
Created attachment 346028 [details]
Patch
Comment 2 Darin Adler 2018-07-29 10:43:09 PDT
Comment on attachment 346028 [details]
Patch

Change seems fine.

Bug title is a bit inaccurate.

Having a constexpr constructor does not affect whether NeverDestroyed is needed. It does affect whether we can have a global outside a function. If the constructor is not constexpr it needs to be inside a function so it gets run the first time the function is installed rather than at load time.

What affects the need for NeverDestroyed is whether the class has a trivial destructor. If it does then NeverDestroyed is not needed.

In all these cases in this patch we changed both things but they are independent.

You can have a static without NeverDestroyed if the destructor is trivial. You can have a global outside a function that uses NeverDestroyed as long as the entire constructor is all constexpr. (Might need to fix NeverDestroyed implementation to be sure it’s constructors are constexpr.)
Comment 3 Darin Adler 2018-07-29 10:50:26 PDT
“Called” not “installed”
Comment 4 Yusuke Suzuki 2018-07-29 11:12:06 PDT
(In reply to Darin Adler from comment #2)
> Comment on attachment 346028 [details]
> Patch
> 
> Change seems fine.
> 
> Bug title is a bit inaccurate.
> 
> Having a constexpr constructor does not affect whether NeverDestroyed is
> needed. It does affect whether we can have a global outside a function. If
> the constructor is not constexpr it needs to be inside a function so it gets
> run the first time the function is installed rather than at load time.
> 
> What affects the need for NeverDestroyed is whether the class has a trivial
> destructor. If it does then NeverDestroyed is not needed.
> 
> In all these cases in this patch we changed both things but they are
> independent.

Right, changed.

> 
> You can have a static without NeverDestroyed if the destructor is trivial.
> You can have a global outside a function that uses NeverDestroyed as long as
> the entire constructor is all constexpr. (Might need to fix NeverDestroyed
> implementation to be sure it’s constructors are constexpr.)

Currently, we cannot have constexpr NeverDestroyed, since it allocates an instance on aligned storage with placement `new`.
This type of code cannot be achieved inside constexpr constructor unfortunately :(
Comment 5 Yusuke Suzuki 2018-07-29 11:13:33 PDT
Committed r234347: <https://trac.webkit.org/changeset/234347>
Comment 6 Radar WebKit Bug Importer 2018-07-29 11:14:17 PDT
<rdar://problem/42710939>