Compilation failure on MinGW in gfx/thebes

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jacek, Assigned: jacek)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

9 years ago
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

9 years ago
Created attachment 389923 [details] [diff] [review]
Fix
(Assignee)

Updated

9 years ago
Attachment #389923 - Flags: review?(vladimir)

Updated

9 years ago
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

9 years ago
Created attachment 405478 [details] [diff] [review]
Use gfxSparseBitSet instead of bitset.

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

9 years ago
Attachment #405478 - Flags: review?(vladimir)
(Assignee)

Updated

9 years ago
Assignee: nobody → jacek
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Blocks: 421095
(Assignee)

Updated

9 years ago
Summary: Compilation failure on MinGW GCC 3.4.6. in gfx/thebes → Compilation failure on MinGW in gfx/thebes
(Assignee)

Comment 3

9 years ago
Created attachment 419252 [details] [diff] [review]
Fix rebased for current version

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

9 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).
(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 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

9 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

9 years ago
Created attachment 419581 [details] [diff] [review]
Fix v1.1

Fixed code style. A superreview is needed due to change in nsTArray.h.
Attachment #419581 - Flags: superreview?(vladimir)
(Assignee)

Comment 10

9 years ago
Created attachment 421068 [details] [diff] [review]
fix v1.2

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 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+
I checked this in to trunk:
http://hg.mozilla.org/mozilla-central/rev/380c1cc8ff84
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.