Closed
Bug 505711
Opened 15 years ago
Closed 15 years ago
Compilation failure on MinGW in gfx/thebes
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jacek, Assigned: jacek)
References
Details
Attachments
(2 files, 3 obsolete files)
4.66 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
5.58 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 Build Identifier: I get following error: gfx/thebes/src/gfxWindowsFonts.cpp:1132: error: conversion from `gfxWindowsFont*' to non-scalar type `nsAutoPtr<gfxWindowsFont>' requested The error is caused by using assignment expression to initialize tempFont. Using a constructor argument fixes the problem. Reproducible: Always
Assignee | ||
Comment 1•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Attachment #389923 -
Flags: review?(vladimir)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Compilation failure on MinGW GCC 3.4.6. → Compilation failure on MinGW GCC 3.4.6. in gfx/thebes
Assignee | ||
Comment 2•15 years ago
|
||
There is one more problem with compiling thebes. Its reason is similar to bug 341128, but I've decided to fix it as suggested by comment and changed the code to use gfxSparseBitSet instead of bitset.
Assignee | ||
Updated•15 years ago
|
Attachment #405478 -
Flags: review?(vladimir)
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → jacek
Status: NEW → ASSIGNED
Assignee | ||
Updated•15 years ago
|
Summary: Compilation failure on MinGW GCC 3.4.6. in gfx/thebes → Compilation failure on MinGW in gfx/thebes
Assignee | ||
Comment 3•15 years ago
|
||
I would really appreciate a review.
Attachment #389923 -
Attachment is obsolete: true
Attachment #405478 -
Attachment is obsolete: true
Attachment #419252 -
Flags: review?(vladimir)
Attachment #389923 -
Flags: review?(vladimir)
Attachment #405478 -
Flags: review?(vladimir)
Comment on attachment 419252 [details] [diff] [review] Fix rebased for current version Redirecting review for font stuff; but, is this working around a too-new compiler or a too-old compiler? Specifically the assignment vs. constructor form change doesn't seem like it should be needed, so if too old compiler I would rather not see workarounds like this in. For the nsTArray.h change, it's probably fine, though what's the problem it's fixing?
Attachment #419252 -
Flags: review?(vladimir) → review?(jfkthame)
Assignee | ||
Comment 5•15 years ago
|
||
Thanks for comments. (In reply to comment #4) > (From update of attachment 419252 [details] [diff] [review]) > Redirecting review for font stuff; but, is this working around a too-new > compiler or a too-old compiler? Specifically the assignment vs. constructor > form change doesn't seem like it should be needed, so if too old compiler I > would rather not see workarounds like this in. I've used old GCC 3.4.6 first and I thought it's a problem with too old compiler, but now I see that the same happens also with GCC 4.4 SVN version, so it seems to be a general problem with GCC. The other problem is similar to bug 341128 (min and max were macros on mingw). Current m-c version seems to no longer suffer from it, but it seems worth fixing anyways (esp. because mingw still declares them as macros). > For the nsTArray.h change, it's probably fine, though what's the problem it's > fixing? It's called from gfxSparseBitSet::test, which I had to change to const because it's called from SupportsLangGroup (that is also const).
Comment 6•15 years ago
|
||
(In reply to comment #5) > > Redirecting review for font stuff; but, is this working around a too-new > > compiler or a too-old compiler? Specifically the assignment vs. constructor > > form change doesn't seem like it should be needed, so if too old compiler I > > would rather not see workarounds like this in. > > I've used old GCC 3.4.6 first and I thought it's a problem with too old > compiler, but now I see that the same happens also with GCC 4.4 SVN version, so > it seems to be a general problem with GCC. I'm not opposed to the change, though I don't understand why it should be needed, as nsAutoPtr<> defines an assignment operator from the raw pointer type. I'm definitely not a C++/template language expert, though. Do you have an explanation for why GCC rejects the existing code? I'm going to mark the patch r+ anyway, but would be interested to learn... Thanks for dealing with the bitset usage; that's been sitting there waiting to be fixed for quite a while.
Comment 7•15 years ago
|
||
Comment on attachment 419252 [details] [diff] [review] Fix rebased for current version A tiny nit: there should be a space after "if" in here: + void set(PRUint32 aIndex, PRBool aValue) { + if(aValue) + set(aIndex);
Attachment #419252 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 8•15 years ago
|
||
Thanks for review, I will attach a new patch with fixed style. (In reply to comment #6) > Do you have an > explanation for why GCC rejects the existing code? I'm going to mark the patch > r+ anyway, but would be interested to learn... I did some tests. The problem is that GCC can't cope with code like this (it simplifies nsAutoPtr behavior): template <class T> class Test { public: class Ptr { public: Ptr(T* aPtr) {} }; Test(Ptr t) {} }; class C { public: C() {} }; int main() { Test<C> test = new C(); } In this case Test<> constructor (not assign operator) is used and it requires Ptr instead of C* pointer and it has to use Ptr constructor to cast it implicitly. GCC can't compile it (tested with Linux GCC 4.4.2 and MinGW GCC 4.4 SVN), but MSC can. I don't know which compiler is right. This problems happens only if you use such expression as constructor. Otherwise an assignment operator is used, which doesn't require a conversion to Ptr.
Assignee | ||
Comment 9•15 years ago
|
||
Fixed code style. A superreview is needed due to change in nsTArray.h.
Attachment #419581 -
Flags: superreview?(vladimir)
Assignee | ||
Comment 10•15 years ago
|
||
Rebased against current m-c
Attachment #419581 -
Attachment is obsolete: true
Attachment #421068 -
Flags: superreview?(vladimir)
Attachment #419581 -
Flags: superreview?(vladimir)
Attachment #421068 -
Flags: superreview?(vladimir) → superreview?(jfkthame)
Comment 11•15 years ago
|
||
Comment on attachment 421068 [details] [diff] [review] fix v1.2 Confirming r+ for this; Vlad said on irc that no separate sr needed.
Attachment #421068 -
Flags: superreview?(jfkthame) → review+
Comment 12•15 years ago
|
||
I checked this in to trunk: http://hg.mozilla.org/mozilla-central/rev/380c1cc8ff84
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•