Bug 177892 – JSC: use default_construct_at

• Status: RESOLVED
• Resolution: WONTFIX
• Component: JavaScriptCore

[JF Bastien]

I added default_construct_at to StdLibExtra in #177473 because some code was otherwise hard to read. I wanted to see how it would feel in other parts of the code, and whether it was a widespread thing that we'd use, so I grep'd for a few places in JSC where I could use it, and did so!

I'm thinking about proposing this for C++20, so getting a feel for it seems useful, and in this case causes very little churn.

[JF Bastien]

Created attachment 322707 [details]
patch

Does it look nicer? More understandable?

[JF Bastien]

Forgot to say: an easy way to grep for likely uses is:

  git grep "new (" ./Source/JavaScriptCore/ | grep -v NotNull | grep "()"

[Saam Barati]

I vote for default_construct_at => construct_at

construct_at should just take variadic parameters it does perfect forwarding on. "Default" flows naturally out of that, e.g, no arguments passed.

[Alexey Proskuryakov]

Comment on attachment 322707 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        It makes code cleaner.

I know how to read placement new, but I would definitely need to look up what default_construct_at or construct_at were.

In general, it seems undesirable to use 1:1 replacements for language constructs, as that adds barriers to entry.

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:91
> -        return new (allocatorPool->alloc(size)) DisjunctionContext();
> +        return &default_construct_at(static_cast<DisjunctionContext*>(allocatorPool->alloc(size)));

This in particular seems harder to read than before.

[Saam Barati]

(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 322707 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322707&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        It makes code cleaner.
> 
> I know how to read placement new, but I would definitely need to look up
> what default_construct_at or construct_at were.
> 
> In general, it seems undesirable to use 1:1 replacements for language
> constructs, as that adds barriers to entry.
> 
> > Source/JavaScriptCore/yarr/YarrInterpreter.cpp:91
> > -        return new (allocatorPool->alloc(size)) DisjunctionContext();
> > +        return &default_construct_at(static_cast<DisjunctionContext*>(allocatorPool->alloc(size)));
> 
> This in particular seems harder to read than before.

I agree. I much prefer placement new.

[JF Bastien]

Agreed with Saam's suggestion, can forward things.

Code such as this is definitely harder to write as well as read:
  new (&container.get()[i]) std::remove_reference_t<decltype(*container.get())>();

That's the original inspiration for the initial proposal.

That being said it's one place, so I don't really care.

[Daniel Bates]

Comment on attachment 322707 [details]
patch

Clearing r and cq flags to get this patch out of these queues.