Weak<> is not cheap feature in terms of memory.
Created attachment 364111 [details] Patch
Comment on attachment 364111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364111&action=review I like this change a lot. I would r=me it but I am not yet an official reviewer. > Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:288 > +#undef DEFINE_BUILTIN_EXECUTABLES So we were undef-ing the wrong macro?! Good catch.
Comment on attachment 364111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364111&action=review > Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:264 > + for (UnlinkedFunctionExecutable*& executable : m_executables) { I know other people don’t agree, but I really prefer "auto&" in a case like this. > Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:273 > + return SourceCode(m_combinedSourceProvider.copyRef(), s_##name - s_JSCCombinedCode, (s_##name - s_JSCCombinedCode) + length, 1, 1);\ The SourceCode { } syntax might be better to use here, giving slightly stricter type checking. And possibly just { } without the explicit SourceCode. > Source/JavaScriptCore/builtins/BuiltinExecutables.h:40 > +#define PUT_BUILTIN_CODE_ID(name, functionName, overriddenName, length) name, Do we want to also include an #undef for this one? Maybe a shorter name for the macro too, like BUILTIN_NAME_ONLY. > Source/JavaScriptCore/builtins/BuiltinExecutables.h:64 > + void finalizeUnconditionally(); > +private: Typically we leave a blank line before the private section.
Comment on attachment 364111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364111&action=review >> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:264 >> + for (UnlinkedFunctionExecutable*& executable : m_executables) { > > I know other people don’t agree, but I really prefer "auto&" in a case like this. In think case, I would personally use the full type as Yusuke did. I like using auto when the type isn't completely clear from the context. Here, it's not clear that the executables are UnlinkedFunctionExecutables rather than say ScriptExecutable. In a hypothetical world where indexing didn't take forever, I'd agree that the type shouldn't be needed. I should be able to ask my editor to get that information from the compiler. Alas, that world doesn't exist yet... :(
(In reply to Keith Miller from comment #4) > Comment on attachment 364111 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=364111&action=review > > >> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:264 > >> + for (UnlinkedFunctionExecutable*& executable : m_executables) { > > > > I know other people don’t agree, but I really prefer "auto&" in a case like this. > > In think case, I would personally use the full type as Yusuke did. I like > using auto when the type isn't completely clear from the context. Here, it's > not clear that the executables are UnlinkedFunctionExecutables rather than > say ScriptExecutable. > > In a hypothetical world where indexing didn't take forever, I'd agree that > the type shouldn't be needed. I should be able to ask my editor to get that > information from the compiler. Alas, that world doesn't exist yet... :( I’m going to try to not rehash the argument, but I will point out, once again, that we seem perfectly able to call functions and use the function results to call a member function without naming the type of each function result. I don’t think that a for loop value is different in this respect, unless the logic inside the loop is complex. It’s not as simple for me as "can you look up the type easily?".
Comment on attachment 364111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364111&action=review >>>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:264 >>>> + for (UnlinkedFunctionExecutable*& executable : m_executables) { >>> >>> I know other people don’t agree, but I really prefer "auto&" in a case like this. >> >> In think case, I would personally use the full type as Yusuke did. I like using auto when the type isn't completely clear from the context. Here, it's not clear that the executables are UnlinkedFunctionExecutables rather than say ScriptExecutable. >> >> In a hypothetical world where indexing didn't take forever, I'd agree that the type shouldn't be needed. I should be able to ask my editor to get that information from the compiler. Alas, that world doesn't exist yet... :( > > I’m going to try to not rehash the argument, but I will point out, once again, that we seem perfectly able to call functions and use the function results to call a member function without naming the type of each function result. I don’t think that a for loop value is different in this respect, unless the logic inside the loop is complex. > > It’s not as simple for me as "can you look up the type easily?". Personally, I like writing full-types as long as the type is meaningful (like, I don't like writing std::vector<int>::const_iterator etc.). So, maybe, `m_unlinkedExecutables` and `unlinkedExecutable` variable names would allow us to use `auto*&`, while keeping its type clear :) Fixed. >> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:273 >> + return SourceCode(m_combinedSourceProvider.copyRef(), s_##name - s_JSCCombinedCode, (s_##name - s_JSCCombinedCode) + length, 1, 1);\ > > The SourceCode { } syntax might be better to use here, giving slightly stricter type checking. And possibly just { } without the explicit SourceCode. Fixed. >> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:288 >> +#undef DEFINE_BUILTIN_EXECUTABLES > > So we were undef-ing the wrong macro?! Good catch. :) >> Source/JavaScriptCore/builtins/BuiltinExecutables.h:40 >> +#define PUT_BUILTIN_CODE_ID(name, functionName, overriddenName, length) name, > > Do we want to also include an #undef for this one? Maybe a shorter name for the macro too, like BUILTIN_NAME_ONLY. Yeah, fixed and changed. >> Source/JavaScriptCore/builtins/BuiltinExecutables.h:64 >> +private: > > Typically we leave a blank line before the private section. Fixed.
Committed r242722: <https://trac.webkit.org/changeset/242722>
<rdar://problem/48775996>