Bug 29024 - Make JavaScriptCore compile on platforms with case-insensitive file systems and typeinfo.h in STL
Summary: Make JavaScriptCore compile on platforms with case-insensitive file systems a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-08 01:36 PDT by Kent Hansen
Modified: 2009-09-09 08:01 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (8.29 KB, patch)
2009-09-08 01:43 PDT, Kent Hansen
mrowe: review-
Details | Formatted Diff | Diff
Revised patch based on comments (12.87 KB, patch)
2009-09-08 03:13 PDT, Kent Hansen
no flags Details | Formatted Diff | Diff
updated patch with proper svn copy (13.52 KB, patch)
2009-09-09 03:37 PDT, Simon Hausmann
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Hansen 2009-09-08 01:36:13 PDT
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.
Comment 1 Kent Hansen 2009-09-08 01:43:26 PDT
Created attachment 39175 [details]
Proposed patch
Comment 2 Mark Rowe (bdash) 2009-09-08 01:55:22 PDT
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.
Comment 3 Kent Hansen 2009-09-08 02:31:25 PDT
(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.
Comment 4 Kent Hansen 2009-09-08 03:13:23 PDT
Created attachment 39178 [details]
Revised patch based on comments
Comment 5 Eric Seidel (no email) 2009-09-08 09:28:18 PDT
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.
Comment 6 Darin Adler 2009-09-08 09:30:07 PDT
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.
Comment 7 Simon Hausmann 2009-09-09 03:37:42 PDT
Created attachment 39259 [details]
updated patch with proper svn copy
Comment 8 Simon Hausmann 2009-09-09 08:01:22 PDT
Committed in revision 48207.