Summary: | Combine SymbolTable and SharedSymbolTable | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | barraclough, buildbot, eflews.bot, ggaren, gtk-ews, gyuyoung.kim, mark.lam, mhahnenberg, msaboff, oliver, philn, rniwa, sam, xan.lopez | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 124760 | ||||||||||
Attachments: |
|
Description
Filip Pizlo
2013-11-21 21:30:00 PST
Created attachment 217651 [details]
the patch
Created attachment 217652 [details]
the patch
Comment on attachment 217652 [details] the patch Attachment 217652 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/33728010 Comment on attachment 217652 [details] the patch Attachment 217652 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/33608018 Comment on attachment 217652 [details] the patch Attachment 217652 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/33718011 Comment on attachment 217652 [details] the patch Attachment 217652 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/33618006 Comment on attachment 217652 [details] the patch Attachment 217652 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/33948001 Comment on attachment 217652 [details] the patch Attachment 217652 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/33778006 Comment on attachment 217652 [details]
the patch
Approach looks good to me, but build is broken:
Undefined symbols for architecture x86_64:
"__ZNK3JSC7JSValuecvPMS0_PvEv", referenced from:
__ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o
__ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o
"__ZNK3JSC7JSValue6asCellEv", referenced from:
__ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o
__ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o
Maybe you need to #include JSValueInlines.h?
(In reply to comment #9) > (From update of attachment 217652 [details]) > Approach looks good to me, but build is broken: > > Undefined symbols for architecture x86_64: > "__ZNK3JSC7JSValuecvPMS0_PvEv", referenced from: > __ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o > __ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o > "__ZNK3JSC7JSValue6asCellEv", referenced from: > __ZN3JSC11SymbolTableC1ERNS_2VME in SymbolTable.o > __ZN3JSC11SymbolTableC2ERNS_2VME in SymbolTable.o Yeah I have a fix locally, just haven't uploaded it yet. > > Maybe you need to #include JSValueInlines.h? We shouldn't ever include that file. The correct thing to do is to make sure that every .cpp file includes "Operations.h", which pulls in a bunch of these BlahInlines.h files. I had forgotten to add such an include to SymbolTable.cpp. (I did a refactoring about a year ago that made Opertions.h work this way. It was because Ossy and I ended up reaching the end of the line for JSC's include dependency graph - there was a refactoring for which no valid #include order in VM.h existed; one variant would break Darwin and another would break everything else. It was impossible to work out why crap broke. So with a bunch of help I moved a lot of inline methods into Inlines.h files, but that created a situations where we would have to include like three different BlahInlines.h files in every .cpp file. So I instituted the "Operations.h is your friend" rule, so that if you include that, you get the inlines you need.) > > Maybe you need to #include JSValueInlines.h?
>
> We shouldn't ever include that file. The correct thing to do is to make sure that every .cpp file includes "Operations.h", which pulls in a bunch of these BlahInlines.h files.
Sounds like an OK rule. This kind of rule would be easier to communicate if we had a file with a clearer name. Maybe JSCInlines.h? We could either factor out the #includes from Operations.h's into a new file, or we could rename Operations.h to JSCInlines.h.
Created attachment 217710 [details]
the patch
Comment on attachment 217710 [details]
the patch
r=me
Landed in http://trac.webkit.org/changeset/159713 |