NeverDestroyed<T> objects should always use direct initialization

• Component: New Bugs
• Status: NEW ---

[Zan Dobersek]

NeverDestroyed<T> objects should always use direct initialization

[Zan Dobersek]

Created attachment 252059 [details]
Patch

[Darin Adler]

Comment on attachment 252059 [details]
Patch

I don’t think we should make this change. Is there one case you can point to here where the different syntax results in different code?

[Darin Adler]

Comment on attachment 252059 [details]
Patch

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

/Volumes/Data/EWS/WebKit/Source/WebCore/platform/network/ios/QuickLook.mm:62:45: error: no viable conversion from 'NSSet *' to 'NeverDestroyed<RetainPtr<NSSet> >'
    static NeverDestroyed<RetainPtr<NSSet>> set = QLPreviewGetSupportedMIMETypes();
                                            ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> Source/WTF/wtf/Noncopyable.h:33
> +// FIXME: Move to a more generic place together with WTF_MAKE_NONCOPYABLE.
> +#define WTF_MAKE_NONMOVABLE(ClassName) \
> +    private: \
> +        ClassName(ClassName&&) = delete; \
> +        ClassName& operator=(ClassName&&) = delete; \

I think we should just write out the "= delete" for both this and non copyable. The macros date back to when this was much trickier to do in the language.

> Source/WebCore/html/HTMLBodyElement.cpp:143
> +    static NeverDestroyed<EventHandlerNameMap> map(createWindowEventHandlerNameMap());

This syntax looks like a function call to me. I prefer the modern syntax with { } if we insist on not using initialization style. But I think I don’t agree with the overall mission here. Have we ever had a real problem with this?

[Zan Dobersek]

(In reply to comment #2)
> Comment on attachment 252059 [details]
> Patch
> 
> I don’t think we should make this change. Is there one case you can point to
> here where the different syntax results in different code?

There's apparently a guarantee that the copy initialization of a NeverDestroyed object with a to-be-wrapped object is optimized into a direct initialization as long as it's possible to convert the to-be-wrapped object into a NeverDestroyed object (which it is, due to the non-explicit NeverDestroyed constructor) and the NeverDestroyed class has a usable move constructor (which it does, since it's not deleted). Despite this optimisation, the move constructor actually requires for the to-be-wrapped object to be copyable, making it impossible to use such initialization with non-copyable classes.

I'd like to disable both of those conditions because that would enforce a uniform way of NeverDestroyed initialization. Given that NeverDestroyed is already non-copyable, there's no reason for it to be movable either. I can't see how a NeverDestroyed object could be ever moved around, but I'd prefer a compile-time prevention than relying on reviews to spot such oddities. The explicit constructor would just ensure there's no way of implicitly constructing NeverDestroyed objects.

So while there's no difference in the generated code, I propose the change for the sake of clarity and for preventive measures. This would require a broader cleanup, from making both NeverDestroyed and LazyNeverDestroyed non-movable to using braced initialization wherever NeverDestroyed is used.