505711 - Compilation failure on MinGW in gfx/thebes

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jacek Caban]

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

Comment 1

[Jacek Caban]

Attachment: Fix

Comment 2

[Jacek Caban]

Attachment: 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.

Comment 3

[Jacek Caban]

Attachment: Fix rebased for current version

I would really appreciate a review.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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?

Comment 5

[Jacek Caban]

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

[Jonathan Kew [:jfkthame]]

(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

[Jonathan Kew [:jfkthame]]

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);

Comment 8

[Jacek Caban]

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.

Comment 9

[Jacek Caban]

Attachment: Fix v1.1

Fixed code style. A superreview is needed due to change in nsTArray.h.

Comment 10

[Jacek Caban]

Attachment: fix v1.2

Rebased against current m-c

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 421068 [details] [diff] [review]
fix v1.2

Confirming r+ for this; Vlad said on irc that no separate sr needed.

Comment 12

[Jonathan Kew [:jfkthame]]

I checked this in to trunk:
http://hg.mozilla.org/mozilla-central/rev/380c1cc8ff84