Bug 123057 - Some includes in JSC seem to use an incorrect style
Summary: Some includes in JSC seem to use an incorrect style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-19 00:07 PDT by Alexey Proskuryakov
Modified: 2013-10-19 16:59 PDT (History)
3 users (show)

See Also:

Attachments
proposed patch (4.36 KB, patch)
2013-10-19 00:10 PDT, Alexey Proskuryakov 		no flags Details | Formatted Diff | Diff
with include order fix (4.41 KB, patch)
2013-10-19 00:14 PDT, Alexey Proskuryakov 		ggaren: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2013-10-19 00:07:33 PDT 
What does #include <heap/Weak.h> even mean inside JSC?
Comment 1 Alexey Proskuryakov 2013-10-19 00:10:51 PDT 
Created attachment 214642 [details]
proposed patch

FWIW, some of these were introduced as recently as bug 114220, not sure why. Maybe there was a reason?

Let's see if this at least compiles everywhere...
Comment 2 WebKit Commit Bot 2013-10-19 00:11:50 PDT 
Attachment 214642 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSContextRef.cpp', u'Source/JavaScriptCore/API/JSStringRefCF.cpp', u'Source/JavaScriptCore/API/JSValueRef.cpp', u'Source/JavaScriptCore/API/OpaqueJSString.cpp', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/parser/SyntaxChecker.h', u'Source/JavaScriptCore/runtime/WeakGCMap.h']" exit_code: 1
Source/JavaScriptCore/API/JSValueRef.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alexey Proskuryakov 2013-10-19 00:14:00 PDT 
Created attachment 214643 [details]
with include order fix
Comment 4 Geoffrey Garen 2013-10-19 11:56:29 PDT 
I believe we need <> style for any header that will be included into a WebCore file.
Comment 5 Geoffrey Garen 2013-10-19 11:56:40 PDT 
Comment on attachment 214643 [details]
with include order fix

r=me
Comment 6 Alexey Proskuryakov 2013-10-19 12:10:58 PDT 
> I believe we need <> style for any header that will be included into a WebCore file.

Some of these headers are included in WebCore... I'm confused :(
Comment 7 Geoffrey Garen 2013-10-19 12:41:46 PDT 
My understanding is that, on Mac, WebCore includes JavaScriptCore headers through the ForwardingHeaders folder, which is treated as a non-local include path. Hence the <> and the folder prefix. Maybe this has changed somehow?
Comment 8 Alexey Proskuryakov 2013-10-19 16:58:28 PDT 
Looks like here is what we somehow naturally stabilized on:

- JSC API headers are included from other projects as <JavaScriptCore/Foo.h>. This works on Mac because of frameworks, and it works on other platforms because of Source/JavaScriptCore/ForwardingHeaders/JavaScriptCore.

- Other JSC headers are included from other projects as <runtime/Foo.h>. This works because of forwarding headers in those projects, I think.

- JSC headers themselves include other JSC headers as "Foo.h". I couldn't figure out how this works on Mac - perhaps Xcode builds a header map for all JSC private headers? On other platforms, it works because each project adds all JSC subdirectories to search paths.

I have my reservations about the beauty of this scheme (and I could misunderstand something). But now I think that it's good to land this patch for consistency. Also, it will make it easier for me to fix the build in bug 86914.
Comment 9 Alexey Proskuryakov 2013-10-19 16:59:49 PDT 
Committed <http://trac.webkit.org/r157687>.