Bug 29187 - Don't inline ~ListRefPtr() to workaround winscw compiler forward declaration limitation.
Summary: Don't inline ~ListRefPtr() to workaround winscw compiler forward declaration ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Emulator S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 27065 29738
  Show dependency treegraph
 
Reported: 2009-09-11 09:11 PDT by Yongjun Zhang
Modified: 2009-10-01 12:18 PDT (History)
3 users (show)

See Also:


Attachments
Don't inline ~ListRefPtr for winscw compiler, keep it inlined for other compilers. (2.11 KB, patch)
2009-09-11 10:48 PDT, Yongjun Zhang
eric: review-
Details | Formatted Diff | Diff
modify patch to include Eric's comments. (2.31 KB, patch)
2009-09-28 08:18 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff
modify patch to include Eric's comments. (2.54 KB, patch)
2009-09-28 09:26 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yongjun Zhang 2009-09-11 09:11:18 PDT
ListRefPtr is used in FontFamily to embed SharedFontFamily as it's member.  Winscw compiler complains that SharedFontFamily is not defined.
Comment 1 Yongjun Zhang 2009-09-11 10:48:29 PDT
Created attachment 39449 [details]
Don't inline ~ListRefPtr for winscw compiler, keep it inlined for other compilers.
Comment 2 Janne Koskinen 2009-09-11 12:49:55 PDT
(In reply to comment #1)
> Created an attachment (id=39449) [details]
> Don't inline ~ListRefPtr for winscw compiler, keep it inlined for other
> compilers.

This can be solved by not inlining the corresponding constructor and assignement operator from refptr.h ?
Comment 3 Yongjun Zhang 2009-09-11 13:31:42 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=39449) [details] [details]
> > Don't inline ~ListRefPtr for winscw compiler, keep it inlined for other
> > compilers.
> This can be solved by not inlining the corresponding constructor and
> assignement operator from refptr.h ?

No, this is for ListRefPtr which is a different templated class than RefPtr.
Comment 4 Eric Seidel (no email) 2009-09-11 16:49:16 PDT
Comment on attachment 39449 [details]
Don't inline ~ListRefPtr for winscw compiler, keep it inlined for other compilers.

Can you help us understand this limitation more?  Have you filed a bug with the winscw folks?  Do they have a public bug tracker?
Comment 5 Yongjun Zhang 2009-09-11 21:21:25 PDT
(In reply to comment #4)
> (From update of attachment 39449 [details])
> Can you help us understand this limitation more?  Have you filed a bug with the
> winscw folks?  Do they have a public bug tracker?

Sorry I didn't explain it in detail in error description.

~ListRefPtr() has the following line:

RefPtr<T> reaper = this->release();

which calls the ctr RefPtr<T>::RefPtr(T*), where ref() is called.  Winscw compiler tries to resolve T::ref() when ~ListRefPtr() is inlined, and it fails if class T is not defined before that.

I filed a bug to winscw team: https://xdabug001.ext.nokia.com/bugzilla/show_bug.cgi?id=9812 for similar issue in https://bugs.webkit.org/show_bug.cgi?id=28054.

Janne, you are right.  If we don't inline the ctor and assignment operator in RefPtr.h, this problem could be resolved.

There is another patch trying to address the same issue for KeyframeList and AnimationBase (https://bugs.webkit.org/show_bug.cgi?id=29204).  And it could be also resolved by removing inline for PassRefPtr's ctor and assignment operator.

Therefore, if we change PassRefPtr & RefPtr's ctor and assignement operator, we can avoid making changes in other places.

Eric, do you think it is a better approach?
Comment 6 Eric Seidel (no email) 2009-09-15 23:02:20 PDT
Comment on attachment 39449 [details]
Don't inline ~ListRefPtr for winscw compiler, keep it inlined for other compilers.

Maybe I don't understand C++ well enough, but I don't understand how you can have non-inline template code in a header.  I don't see what the "inline" keyword would do here, the destructor will always be inlined, no?

Also, this needs a comment in the code and a link to the winscw bug.

What is being forward declared here that winscw can't understand?
Comment 7 Darin Adler 2009-09-16 08:26:08 PDT
(In reply to comment #6)
> (From update of attachment 39449 [details])
> Maybe I don't understand C++ well enough, but I don't understand how you can
> have non-inline template code in a header.

You're right to doubt your C++ knowledge here, Eric. Template code goes in headers; whether it's marked inline or not is not important. It's not correct to have a non-template function in a header unless it's marked inline, but that does not apply to function templates.
Comment 8 Yongjun Zhang 2009-09-16 11:39:19 PDT
(In reply to comment #6)
Thanks a lot, Eric.

> Also, this needs a comment in the code and a link to the winscw bug.

 

> What is being forward declared here that winscw can't understand?
Comment 9 Yongjun Zhang 2009-09-16 11:58:18 PDT
(In reply to comment #6)
Thanks a lot, Eric.

> Also, this needs a comment in the code and a link to the winscw bug.

I will modify the patch as suggested.

> What is being forward declared here that winscw can't understand?

Class FontFamily (in WebCore/platform/graphics) has member "    ListRefPtr<SharedFontFamily> m_next".  SharedFontFamily is forward declared but winscw compiler complains it is not enough to resolve SharedFontFamily::ref(), when ~ListRefPtr is inlined in the implicit default dtor of FontFamily (which is inlined too).

To summerize how this happens:

1. ~FontFamily() is inlined, the compiler tries to resolve ~ListRefPtr() from inside.
2. ~ListRefPtr() has line "RefPtr<T> reaper = this->release();", 
which calls the ctr RefPtr<T>::RefPtr(T*), where ref() is called.  Winscw
compiler tries to resolve T::ref() when ~ListRefPtr() is inlined, and it fails
if class T is not defined before that.
Comment 10 Yongjun Zhang 2009-09-28 08:18:26 PDT
Created attachment 40235 [details]
modify patch to include Eric's comments.


add link to winscw compiler bug in ChangeLog, to remind us revert this change when the bug is fixed.
Comment 11 Yongjun Zhang 2009-09-28 09:26:01 PDT
Created attachment 40242 [details]
modify patch to include Eric's comments.


document the winscw compiler bug in ChangeLog and the changed source code.
Comment 12 WebKit Commit Bot 2009-10-01 12:18:11 PDT
Comment on attachment 40242 [details]
modify patch to include Eric's comments.

Clearing flags on attachment: 40242

Committed r48988: <http://trac.webkit.org/changeset/48988>
Comment 13 WebKit Commit Bot 2009-10-01 12:18:16 PDT
All reviewed patches have been landed.  Closing bug.