564402 - slots #define in Qt, prevents Qt builds on trunk

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Mike Kristoffersen (:MikeK)]

Due to a #define of "slots" Qt builds are failing, as "slots" are used as a local variable name in nsINode.h

Comment 1

[Mike Kristoffersen (:MikeK)]

Attachment: Undefines the "slots" #define

Comment 2

[Oleg Romashin (:romaxa)]

Patch looks like not a patch... ;)

I think bug 564291 and this bug are duplicating each other

Comment 3

[Mike Kristoffersen (:MikeK)]

Attachment: The real patch

I must have clicked the wrong file in the previous attachment...

Comment 4

[Mike Kristoffersen (:MikeK)]

Comment on attachment 444065 [details] [diff] [review]
The real patch

Wrong file

Comment 5

[Mike Kristoffersen (:MikeK)]

My FF attach file function seems to be seriously broken, The file I click, in the "open file" dialog is not the one that gets selected...

For anyone interested the patch can be found at: http://pastebin.mozilla.org/721855  (can anyone attach it for me, until I figure out what is wrong with my system)

I'll reboot my computer when it finishes the builds its running, to see if that solves the issue....

Comment 7

[Oleg Romashin (:romaxa)]

Attachment: Fix

Comment 8

[Oleg Romashin (:romaxa)]

> It's not the nsSlots that is the problem, its a macro definition of "slots"

hrrr.. I was searching for nsSlots in headers...

We had similar problem before... but that was fixed by renaming variable name from slots->slotss or something like that ;)

Comment 9

[Mike Kristoffersen (:MikeK)]

Yes, I also had kind of a dejavu when I saw this... an unfortunate design decision in the Qt headers...

Comment 10

[Mike Kristoffersen (:MikeK)]

(In reply to comment #8)

> We had similar problem before... but that was fixed by renaming variable name
> from slots->slotss or something like that ;)

btw - I opted for an undefine of slots this time, as I don't think our "generic" code should "suffer" from an old fashioned #define in the Qt headers - we wouldn't even be able to solve this issue with namespaces as #defines are a pure text substitutions...

Comment 11

[Mike Kristoffersen (:MikeK)]

(In reply to comment #7)
> Created an attachment (id=444068) [details]
> Fix

Thank you - seems like I needed a reboot of my PC for the attachment function to work as intended again.

Comment 12

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/6c56e83f9599

Comment 13

[Doug Turner (:dougt)]

hmm.  on second thought, i am not sure I like this.  we should just rename the variables in nsINode.h.

Comment 14

[Oleg Romashin (:romaxa)]

Or start complain about ugly Qt headers... and ask them use namespace

Comment 15

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 16

[Mike Kristoffersen (:MikeK)]

(In reply to comment #14)
> Or start complain about ugly Qt headers... and ask them use namespace

namespace doesn't help for #defines (at least not in the C++ sense), if they didn't use #define we would not have a problem... but I agree the headers are ugly

Comment 17

[Mike Kristoffersen (:MikeK)]

(In reply to comment #13)
> hmm.  on second thought, i am not sure I like this.  we should just rename the
> variables in nsINode.h.

Why should our code "suffer" from purely designed headers?

Comment 18

[Doug Turner (:dougt)]

we can complain all we want, but the Qt ugly is already out in the world.  We have sucked it up elsewhere (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURI.idl#73, and a bunch of other places)

Lets move on.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 444752 [details] [diff] [review]
patch v.2

Bastards.

Comment 20

[Mike Kristoffersen (:MikeK)]

(In reply to comment #17)
> (In reply to comment #13)
> > hmm.  on second thought, i am not sure I like this.  we should just rename the
> > variables in nsINode.h.
> 
> Why should our code "suffer" from purely designed headers?

Let me rephrase this question:

Why should our non-Qt related code suffer from a mistake in a Qt header, when that mistake can be contained to our own Qt specific files?

Comment 21

[Doug Turner (:dougt)]

With your patch, there were other files that stilled failed to compile for the same reason.  Instead of fixing those in a similar manner, I felt it was just easier to rename the local variables used in the public dom interface.

Comment 22

[Mike Kristoffersen (:MikeK)]

(In reply to comment #21)
> With your patch, there were other files that stilled failed to compile for the
> same reason.  Instead of fixing those in a similar manner, I felt it was just
> easier to rename the local variables used in the public dom interface.

Strange it compiled fine for me... - sometimes its better to take the easy solution than the right one - in this case I'm not sure - it is definitively not the first time we have problems with a #define form Qt, and it is probably not the last time either.

Would it be an idea if I create a new header file that gets included in all of our files that includes Qt headers where we can deal with this kind of things?  

We already have an unwritten rule that states that the Qt files needs to be first in the file, I don't think it would be too bad to have a rule that it must be terminated by a specific file too.

First of all when we get these errors it is not obvious what happens by looking at the compiler errors, which makes it more difficult for the poor person who happens to declare a local slots variable.

Secondary it is against all common coding practice to use a #define that way - it will continue to haunt us forever if we don't try to contain it to our Qt files.

And finally we don't have any Qt builds on the FireFox tinderbox, which means that most people don't discover when they break Qt in the first place - we might want to fix that issue in another way thou...

That being said, this is not one of the battles that I'm going to invest much more time in, this is just a minor thing compared to other problems in our codebase.

Comment 23

[Doug Turner (:dougt)]

if you would like to create a new mozilla/qt header file that we include that undefs slots, and then find all places where we #include Qt stuff, go for it.  (separate bug, very low priority)

Comment 24

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/fb38d987711f