Bug 71456 – [PATCH] Move duplicates of SYMBOL_STRING* macros to the single location

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Priit Laes (IRC: plaes)]

Currently the SYMBOL_STRING* macros are defined in three separate places.

[Priit Laes (IRC: plaes)]

Created attachment 113454 [details]
webkit-bug-71456-symbol-string.patch

Smoketested on x86-64 only with release build.

Also, this patch was made after rollout of r99089 in bug #71448

[Gavin Barraclough]

Comment on attachment 113454 [details]
webkit-bug-71456-symbol-string.patch

I have a local change to carve out of a big patch that would do something similar to this, so agree in principal, and would be happy to see something like this land - but I'm not sure that Platform.h is really the right place for things like this to go.

After discussing with Sam, I was going to add a new wtf/InlineASM.h file for helper macros like this, but am open to better suggestions.  I'm going to clear r- for now, so that no-one is too trigger happy on the r+ while we discuss this.

[Priit Laes (IRC: plaes)]

(In reply to comment #2)
> (From update of attachment 113454 [details])
> I have a local change to carve out of a big patch that would do something similar to this, so agree in principal, and would be happy to see something like this land - but I'm not sure that Platform.h is really the right place for things like this to go.
> 
> After discussing with Sam, I was going to add a new wtf/InlineASM.h file for helper macros like this, but am open to better suggestions.  I'm going to clear r- for now, so that no-one is too trigger happy on the r+ while we discuss this.

wtf/InlineASM.h sounds excellent!

[Gavin Barraclough]

Hi Priit, I'm going to make this change, I do hope this doesn't conflict with any changes you have.

[Gavin Barraclough]

Created attachment 114182 [details]
new patch

[Darin Adler]

Why is ASM all capitals?

[Gavin Barraclough]

fixed in r99643

(In reply to comment #6)
> Why is ASM all capitals?

Hrrrm, that was an error, I guess I accidentally treated it as an acronym rather than an abbreviation.

I'm not sure if it's a good idea to fix this now - I could remove InlineASM.h & replace with InlineAsm.h, but might that be problematic on case insensitive file systems?

Alternatively, I could rename - AsmHelpers.h was also in the running, that wouldn't have the same issue.

[Darin Adler]

(In reply to comment #7)
> I'm not sure if it's a good idea to fix this now - I could remove InlineASM.h & replace with InlineAsm.h, but might that be problematic on case insensitive file systems?
> 
> Alternatively, I could rename - AsmHelpers.h was also in the running, that wouldn't have the same issue.

Maybe you could rename it InlineAssembly.h at some point.

[Gavin Barraclough]

Okay, will do.