Bug 231436 – SourceID should have a type name and only be 32-bits

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

SourceID should have a type name and only be 32-bits

[Keith Miller]

Created attachment 440627 [details]
Patch

[Keith Miller]

Created attachment 440637 [details]
Patch

[Mark Lam]

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.

[Keith Miller]

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.

[Keith Miller]

Created attachment 440785 [details]
Patch for landing

[Keith Miller]

Created attachment 440786 [details]
Patch for landing

[Keith Miller]

Created attachment 440791 [details]
Patch for landing

[EWS]

Patch 440786 does not build

[Keith Miller]

Created attachment 440797 [details]
Patch for landing

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/84103788>

[Darin Adler]

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.

[Keith Miller]

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

[Darin Adler]

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.