Bug 203391 – Move JSC::Register inline methods into RegisterInlines.h.

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

[Mark Lam]

.. and handle the fallout of this change.  We're doing this because:
1. RegisterInlines.h is the canonical place to put inline Register methods.
2. Helps reduce build time.
3. This enables experimental work to box JSCells in JSValue.

[Mark Lam]

Created attachment 381865 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 381865 [details]
proposed patch.

r=me if EWS gets green.

[Mark Lam]

Created attachment 381866 [details]
patch for landing.

[Keith Miller]

Comment on attachment 381865 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=381865&action=review

r=me too.

> Source/JavaScriptCore/interpreter/ProtoCallFrameInlines.h:62
> +inline CodeBlock* ProtoCallFrame::codeBlock() const

out of curiosity, do these functions need to be marked inline when the class body declares them as such? I would guess no but idk.

[Mark Lam]

(In reply to Keith Miller from comment #4)
> > Source/JavaScriptCore/interpreter/ProtoCallFrameInlines.h:62
> > +inline CodeBlock* ProtoCallFrame::codeBlock() const
> 
> out of curiosity, do these functions need to be marked inline when the class
> body declares them as such? I would guess no but idk.

I prefer to when I can.  Here's why:

If the header file declares it "inline", and the .cpp file using it forgets to #include the Inlines.h file, the compiler will complain immediately rather than waiting till the Linker fails.  So, we get more immediate feedback, plus it tells the reader to look for the function in the Inlines.h.

However, there are places that I deliberately choose not to add the "inline" modifier in the header file.  Here's why:

Let's say A.h has an inline function foo().
Let's say B.h has an inline function bar() that calls foo.
Let's say C.cpp #includes B.h but does not call bar() anywhere.

If I add the "inline" modifier to foo in A.h, then I'll be forced to #include AInlines.h in C.cpp even though I never call it.  It may also force me to move bar out to a BInlines.h.

But if I don't add the "inline" modifier to foo in A.h, I get too leave B.h and C.cpp alone.

I relied on this detail to save me a lot of work in this patch.

[Mark Lam]

Created attachment 381867 [details]
patch for landing.

[Mark Lam]

Thanks for the reviews.  Landed in r251584: <http://trac.webkit.org/r251584>.

[Radar WebKit Bug Importer]

<rdar://problem/56610023>

[Fujii Hironori]

Filed : Bug 203483 – [Windows][Clang] error LNK2001: unresolved external symbol "void * __cdecl JSC::allocateCell<class JSC::JSGenericTypedArrayView<struct JSC::Float32Adaptor> >(class JSC::Heap &,unsigned __int64)"

[Fujii Hironori]

(In reply to Mark Lam from comment #5)
> However, there are places that I deliberately choose not to add the "inline"
> modifier in the header file.  Here's why:
> 
> Let's say A.h has an inline function foo().
> Let's say B.h has an inline function bar() that calls foo.
> Let's say C.cpp #includes B.h but does not call bar() anywhere.
> 
> If I add the "inline" modifier to foo in A.h, then I'll be forced to
> #include AInlines.h in C.cpp even though I never call it.  It may also force
> me to move bar out to a BInlines.h.

In that case, B.h includes A.h, and C.cpp includes just only B.h.

If A.h is using B's methods, AInlines.h and BInlines.h are needed.
AInlines.h includes A.h and B.h, and BInlines.h includes AInlines.h.
C.cpp needs to include just only BInlines.h.
There is no disadvantage for adding 'inline' specifier.

https://webkit.org/code-style-guidelines/#include-primary

Furthermore, it confirms header's completeness of WebKit style guideline.

[Mark Lam]

(In reply to Fujii Hironori from comment #10)
> (In reply to Mark Lam from comment #5)
> > However, there are places that I deliberately choose not to add the "inline"
> > modifier in the header file.  Here's why:
> > 
> > Let's say A.h has an inline function foo().
> > Let's say B.h has an inline function bar() that calls foo.
> > Let's say C.cpp #includes B.h but does not call bar() anywhere.
> > 
> > If I add the "inline" modifier to foo in A.h, then I'll be forced to
> > #include AInlines.h in C.cpp even though I never call it.  It may also force
> > me to move bar out to a BInlines.h.
> 
> In that case, B.h includes A.h, and C.cpp includes just only B.h.
> 
> If A.h is using B's methods, AInlines.h and BInlines.h are needed.
> AInlines.h includes A.h and B.h, and BInlines.h includes AInlines.h.
> C.cpp needs to include just only BInlines.h.
> There is no disadvantage for adding 'inline' specifier.
> 
> https://webkit.org/code-style-guidelines/#include-primary
> 
> Furthermore, it confirms header's completeness of WebKit style guideline.

My point, in this example, is that BInlines.h doesn't exists yet, and creating it will change gazillion files ... an investment I didn't want to take at this point.  Hence, by leaving the "inline" modifier out, I avoided having to make this change ... since the inline function isn't actually in use in C.cpp.

[Fujii Hironori]

C.cpp don't need to include BInlines.h if it is not using bar() nor foo().

Adding 'inline' specifier doesn't force you to define the definition.

https://godbolt.org/z/vTwSNx

[Mark Lam]

(In reply to Fujii Hironori from comment #12)
> C.cpp don't need to include BInlines.h if it is not using bar() nor foo().
> 
> Adding 'inline' specifier doesn't force you to define the definition.
> 
> https://godbolt.org/z/vTwSNx

Hi Fujii, I modified your example to reflect the scenario I was talking about, and it reproduces the build failure.  Here it is:

https://godbolt.org/z/Z-TVrd

Thanks.

[Fujii Hironori]

Yeah, that's the case you mentioned in your ChangeLog entry.

> This patch also handles the fallout of this change, which necessitates more
> inline methods being moved from <file>.h to their respective <file>Inlines.h.
> 
> JSArray.h used to include ButterflyInlines.h and JSCellInlines.h.  This is a
> violation of inclusion ordering (.h should not #include Inlines.h).  This
> violation has been removed.