...
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>