Bug 29106 - [Qt] make CachedResourceHandle.h compile in winscw Symbian compiler.
Summary: [Qt] make CachedResourceHandle.h compile in winscw Symbian compiler.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Emulator S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 27065
  Show dependency treegraph
 
Reported: 2009-09-09 14:45 PDT by Yongjun Zhang
Modified: 2009-10-13 22:24 PDT (History)
4 users (show)

See Also:


Attachments
don't inline CachedResourceHandle<T>(R*) for winscw compiler. (2.80 KB, patch)
2009-09-09 21:23 PDT, Yongjun Zhang
eric: review-
Details | Formatted Diff | Diff
modify patch to include Eric's comments. (3.14 KB, patch)
2009-09-30 14:25 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-09 14:45:51 PDT
In Winscw compiler, when CachedResourceHandle<T> is used as a class member,  the compiler will aggressively resolve the inheritance of R* when constructor CachedResourceHandle(R* res) is inlined, which means the class definition of T must come before CachedResourceHandle<T> and the header file of class T must be included wherever CachedResourceHandler<T> is defined.

To avoid unnecessary inclusion of header files, the right workaround would be removing inline for CacheResourceHandler<T>(R*) in winscw compiler, while keep it inline for other compilers to avoid performance penalty.
Comment 1 Yongjun Zhang 2009-09-09 21:23:20 PDT
Created attachment 39320 [details]
don't inline CachedResourceHandle<T>(R*) for winscw compiler.
Comment 2 Ariya Hidayat 2009-09-10 03:13:32 PDT
Nitpick: technically [Qt] is not needed, right?
Comment 3 Eric Seidel (no email) 2009-09-10 12:45:22 PDT
Comment on attachment 39320 [details]
don't inline CachedResourceHandle<T>(R*) for winscw compiler.

I'm not sure how it makes sense to ever have a non-inline constructor definition in a header file. :)
Comment 4 Simon Hausmann 2009-09-10 13:39:29 PDT
From what I understand from Janne the outlining shouldn't be necessary if RefPtr is patched appropriately. Janne, can you elaborate? :)
Comment 5 Janne Koskinen 2009-09-10 14:05:01 PDT
Right, I'll post my patch first thing in the morning. So we can fight it over tomorrow :)
Comment 6 Yongjun Zhang 2009-09-10 20:02:52 PDT
(In reply to comment #4)
> From what I understand from Janne the outlining shouldn't be necessary if
> RefPtr is patched appropriately. Janne, can you elaborate? :)

RefPtr is not related here. 

The root cause is the base constructor requires CachedResource* argument.  When CachedResourceHandle<T>(R*) is used, the compiler tries to convert R to CachedResource in the base class's ctor, which fails compiling when class R's header file is not included.  Making it not inline can defer the type conversion till it is really needed.
Comment 7 Janne Koskinen 2009-09-11 03:31:36 PDT
> RefPtr is not related here. 
> 
> The root cause is the base constructor requires CachedResource* argument.  When
> CachedResourceHandle<T>(R*) is used, the compiler tries to convert R to

Right you are. I had a fix in CachedImage that I had forgotten. Putting the fix into base class is better.

Those who still are sceptical:
mwccsym2 tries to resolve inlines immediately and here if the constructor is not taken out would give "illegal use of incomplete struct/union/class" - error as the actual class is not yet been defined within the compilation unit, only forward declared.
Comment 8 Eric Seidel (no email) 2009-09-15 23:15:33 PDT
Comment on attachment 39320 [details]
don't inline CachedResourceHandle<T>(R*) for winscw compiler.

See my comments on your other bug about "inline" for templates functions.

Style:
+        CachedResourceHandle(R* res);

We need a comment which references the bug.  The compiler bug should be linked to from the ChangeLog too.
Comment 9 Yongjun Zhang 2009-09-30 14:25:43 PDT
Created attachment 40399 [details]
modify patch to include Eric's comments.


Added links to winscw compiler bug in ChangeLog and header file.
Comment 10 WebKit Commit Bot 2009-10-13 07:27:40 PDT
Comment on attachment 40399 [details]
modify patch to include Eric's comments.

Rejecting patch 40399 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=40399 from bug 29106 failed to download and apply.
Comment 11 Laszlo Gombos 2009-10-13 16:39:53 PDT
Comment on attachment 40399 [details]
modify patch to include Eric's comments.

Will commit manually; patch does not apply cleanly.
Comment 12 Laszlo Gombos 2009-10-13 22:24:14 PDT
Landed as http://trac.webkit.org/changeset/49556. - part of it was landed previously as http://trac.webkit.org/changeset/49017.