Bug 216193

Created attachment 408039 [details]
Patch

Comment on attachment 408039 [details]
Patch

Will update

Created attachment 408042 [details]
Patch

Comment on attachment 408042 [details]
Patch

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

This all looks good, but I’d like to wait until EWS tests are passing and I’ll review then if no one has done it yet.

> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:114
> +    IntlDateTimeFormat* dtf = IntlDateTimeFormat::unwrapForOldFunctions(globalObject, callFrame->thisValue());

How about "format" instead of "dtf"? And auto perhaps?

    auto format = ...

> Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:117
> +    IntlNumberFormat* nf = IntlNumberFormat::unwrapForOldFunctions(globalObject, callFrame->thisValue());

auto format =

Comment on attachment 408042 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:114
>> +    IntlDateTimeFormat* dtf = IntlDateTimeFormat::unwrapForOldFunctions(globalObject, callFrame->thisValue());
> 
> How about "format" instead of "dtf"? And auto perhaps?
> 
>     auto format = ...

Changed to auto*.

>> Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:117
>> +    IntlNumberFormat* nf = IntlNumberFormat::unwrapForOldFunctions(globalObject, callFrame->thisValue());
> 
> auto format =

Changed to `auto*`. It is good that we know this is a pointer for readability.

Created attachment 408057 [details]
Patch

Updated.

Created attachment 408058 [details]
Patch

Comment on attachment 408058 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:114
> +    auto* dtf = IntlDateTimeFormat::unwrapForOldFunctions(globalObject, callFrame->thisValue());

auto format

> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:146
> +    auto* dateTimeFormat = jsDynamicCast<IntlDateTimeFormat*>(vm, callFrame->thisValue());

I’d like auto format here too

> Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:117
> +    auto* nf = IntlNumberFormat::unwrapForOldFunctions(globalObject, callFrame->thisValue());

auto format

Comment on attachment 408042 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:117
>>> +    IntlNumberFormat* nf = IntlNumberFormat::unwrapForOldFunctions(globalObject, callFrame->thisValue());
>> 
>> auto format =
> 
> Changed to `auto*`. It is good that we know this is a pointer for readability.

Part of my suggestion was "format" instead of "nf".

Also, I strongly suggest "auto" instead of "auto*" so we can change return values to smart pointers without touching all the code. There are plenty of things that act like pointers that don’t work with auto*.

Committed r266655: <https://trac.webkit.org/changeset/266655>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408058 [details].

<rdar://problem/68381902>

Comment on attachment 408042 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:117
>>>> +    IntlNumberFormat* nf = IntlNumberFormat::unwrapForOldFunctions(globalObject, callFrame->thisValue());
>>> 
>>> auto format =
>> 
>> Changed to `auto*`. It is good that we know this is a pointer for readability.
> 
> Part of my suggestion was "format" instead of "nf".
> 
> Also, I strongly suggest "auto" instead of "auto*" so we can change return values to smart pointers without touching all the code. There are plenty of things that act like pointers that don’t work with auto*.

I think auto* in JavaScriptCore (not in WebCore) has more meaning than `auto*` in WebCore. The reason is that, in JSC, we are having GC-managed JSCell derived objects. And we are using it via raw pointer. So, `auto*` in JSC tells us that this is likely this value is GC-managed JSCell.
For non-JSCell things, possibly using `auto` is better. But for JSCell cases, I think `auto*` is nice for readability. When reading the code, `auto` cannot offer information about the ownership management (if it is JSCell, it is GC-managed, and this is largely different from the other C++ objects). But `auto*` can tell us that this is a raw pointer, so we should care about ownership carefully, if it is JSCell, we should consider it as GC-managed. And if it is not JSCell, then we should be careful about the ownership of this raw pointer.

Committed r266679: <https://trac.webkit.org/changeset/266679>