Bug 29024 – Make JavaScriptCore compile on platforms with case-insensitive file systems and typeinfo.h in STL

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Kent Hansen]

JavaScriptCore/runtime/TypeInfo.h clashes with standard header typeinfo.h on certain platforms with case-insensitive file system.
typeinfo.h is being included from another standard header (that we need to include), so we can't change it.
The proposed fix is to rename TypeInfo.h to JSTypeInfo.h.

[Kent Hansen]

Created attachment 39175 [details]
Proposed patch

[Mark Rowe (bdash)]

Comment on attachment 39175 [details]
Proposed patch

When renaming a file like this you should update all references to the file so that you don't break the build.

A few other points:
1) Includes should be kept in alphabetical order.
2) The JS prefix is typically reserved for objects that represent an object within the JavaScript environment (JSArray, JSString, etc).  Maybe this case is ok for the same reason that JSType is ok.
3) Classes live inside files of the same name.  Since you're renaming the file I suspect that you should rename the class to match.
4) You should explicit about which platform this is intended to fix.  It's quite clear that JavaScriptCore compiles on Mac OS X and Windows, both of which use case-insensitive file systems.  If roll out this change in the future and only test on those platforms, we'll break whatever platform it is you're trying to fix here.

[Kent Hansen]

(In reply to comment #2)
> (From update of attachment 39175 [details])
> When renaming a file like this you should update all references to the file so
> that you don't break the build.

Will fix.

> A few other points:
> 1) Includes should be kept in alphabetical order.

Will fix.

> 2) The JS prefix is typically reserved for objects that represent an object
> within the JavaScript environment (JSArray, JSString, etc).  Maybe this case is
> ok for the same reason that JSType is ok.
> 3) Classes live inside files of the same name.  Since you're renaming the file
> I suspect that you should rename the class to match.

Renaming the class itself would result in a much larger change.
Is there a better name we can use to avoid hi-jacking the JS prefix?
Renaming the file is the same solution as that already used for WebCore/platform/text/PlatformString.h; see comment near top of file. I'll update the patch to include a similar comment in JSTypeInfo.h.

> 4) You should explicit about which platform this is intended to fix.  It's
> quite clear that JavaScriptCore compiles on Mac OS X and Windows, both of which
> use case-insensitive file systems.  If roll out this change in the future and
> only test on those platforms, we'll break whatever platform it is you're trying
> to fix here.

I've updated the summary and will update the ChangeLog to be more explicit about this.

[Kent Hansen]

Created attachment 39178 [details]
Revised patch based on comments

[Eric Seidel (no email)]

Comment on attachment 39178 [details]
Revised patch based on comments

This looks OK to me, but since Mark reviewed the first patch, I would like to leave him a chance to comment on this one as well.  He can mark it r+/cq+ if he likes what he sees.

[Darin Adler]

Comment on attachment 39178 [details]
Revised patch based on comments

To preserve the file history, you should use "svn mv" instead of just removing the old file and adding the new one.

[Simon Hausmann]

Created attachment 39259 [details]
updated patch with proper svn copy

[Simon Hausmann]

Committed in revision 48207.