SourceID should have a type name and only be 32-bits
Created attachment 440627 [details] Patch
Created attachment 440637 [details] Patch
Comment on attachment 440637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440637&action=review r=me also with fix. > Source/JavaScriptCore/debugger/DebuggerPrimitives.h:33 > -using SourceID = size_t; > +using SourceID = uint32_t; > constexpr SourceID noSourceID = 0; Can we put these in a SourceID.h or perhaps an ID.h if we want to put other IDs (like the BreakpointID and BreakpointActionID below) in it as well? Looks like we need this declaration / definition in more than 1 place. It's best to have one source of truth instead of having code duplication. > Source/JavaScriptCore/interpreter/StackVisitor.h:49 > +using SourceID = uint32_t; nit: code duplication like this is error prone. Can we #include a SourceID.h instead? > Source/JavaScriptCore/parser/SourceCode.h:35 > +using SourceID = uint32_t; Duplication again. > Source/JavaScriptCore/parser/SourceCode.h:37 > +class SourceCode : public UnlinkedSourceCode { I presume the only change for this code is just the un-indent + main providerID() return SourceID. > Source/JavaScriptCore/parser/SourceProvider.h:51 > + using SourceID = uint32_t; Again! > Source/JavaScriptCore/runtime/FunctionHasExecutedCache.h:34 > +using SourceID = uint32_t; Ditto.
Comment on attachment 440637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440637&action=review m >> Source/JavaScriptCore/debugger/DebuggerPrimitives.h:33 >> constexpr SourceID noSourceID = 0; > > Can we put these in a SourceID.h or perhaps an ID.h if we want to put other IDs (like the BreakpointID and BreakpointActionID below) in it as well? Looks like we need this declaration / definition in more than 1 place. It's best to have one source of truth instead of having code duplication. Yeah, I can put it in the new Forward.h at least until it becomes a real class or something. On the other hand, I don't think it's as error prone as you think as non-equilevent definitions are a compiler error. >> Source/JavaScriptCore/parser/SourceCode.h:37 >> +class SourceCode : public UnlinkedSourceCode { > > I presume the only change for this code is just the un-indent + main providerID() return SourceID. Yeah, I'm so used to classes being on the first line I get confused these days so I've started un-indenting them in clean up patches. I'll add a note to the changelog too.
Created attachment 440785 [details] Patch for landing
Created attachment 440786 [details] Patch for landing
Created attachment 440791 [details] Patch for landing
Patch 440786 does not build
Created attachment 440797 [details] Patch for landing
Committed r283903 (242778@main): <https://commits.webkit.org/242778@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440797 [details].
<rdar://problem/84103788>
Comment on attachment 440797 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=440797&action=review > Source/JavaScriptCore/runtime/TypeProfiler.h:61 > + : m_sourceID(UINT_MAX) This seems to defeat the purpose of using a named type. I would have suggested std::numeric_limits<SourceID>::max(). > Source/JavaScriptCore/runtime/TypeProfiler.h:70 > - return m_sourceID == INTPTR_MAX > + return m_sourceID == UINT_MAX > && m_divot == UINT_MAX > && m_searchDescriptor == TypeProfilerSearchDescriptorFunctionReturn; Seems like this could check just m_sourceID; no reason to check the other two.
Comment on attachment 440797 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=440797&action=review >> Source/JavaScriptCore/runtime/TypeProfiler.h:61 >> + : m_sourceID(UINT_MAX) > > This seems to defeat the purpose of using a named type. I would have suggested std::numeric_limits<SourceID>::max(). Fair enough. Do you think it's worth making a new patch for this? >> Source/JavaScriptCore/runtime/TypeProfiler.h:70 >> && m_searchDescriptor == TypeProfilerSearchDescriptorFunctionReturn; > > Seems like this could check just m_sourceID; no reason to check the other two. Yeah, I agree. I just refactored the existing code without thinking too hard...
Comment on attachment 440797 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=440797&action=review >>> Source/JavaScriptCore/runtime/TypeProfiler.h:61 >>> + : m_sourceID(UINT_MAX) >> >> This seems to defeat the purpose of using a named type. I would have suggested std::numeric_limits<SourceID>::max(). > > Fair enough. Do you think it's worth making a new patch for this? Sorry, I don’t understand the uestion.