573210 - Consistently qualify accesses to dependent base classes in xpcom C++ code

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eli Friedman]

Attachment: Patch

Per summary; clang (http://clang.llvm.org/) is more strict about lookup in dependent base classes than other C++ compilers.  Patch attached.

First patch in a series of patches to allow compiling Firefox with clang (see http://llvm.org/bugs/show_bug.cgi?id=5511).

Comment 1

[Benjamin Smedberg]

Comment on attachment 452444 [details] [diff] [review]
Patch

This is ugly. If necessary, introduce a self_type typedef.

Comment 2

[Eli Friedman]

Attachment: Patch v2

I was trying to match local style, but if you prefer something more like this...

Comment 3

[Benjamin Smedberg]

Comment on attachment 452476 [details] [diff] [review]
Patch v2

dbaron, can you check this? It seems to me that clang is being incorrectly strict, since it can assume that identifiers which are not qualified with `typename` are dependent on the base type.

Comment 4

[Benjamin Smedberg]

cc'ing my compiler experts

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This seems plausible based on rumors I've heard about name lookup in templates, but I really don't know much about that part of the C++ spec.  Any idea if the gcc folks think it's a bug that they accept this code?

Comment 6

[Eli Friedman]

See http://clang.llvm.org/cxx_compatibility.html#dep_lookup_bases .  And the gcc PR is http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24163 .

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why are you prefering base_type::method() rather than this->method() ?  It seems like I'd default to preferring the latter unless you have a reason for the former.

Comment 8

[Eli Friedman]

I wasn't really thinking about it; I'll change it to this->method().

Comment 9

[Eli Friedman]

Attachment: Patch v3

Updated to prefer "this->".

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 453300 [details] [diff] [review]
Patch v3

r=dbaron

Do you (still?) have commit access, or do you need someone to land this for you?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(If you need/want someone to land it for you, you can add the 'checkin-needed' keyword.)

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/2c778979f15a