Closed Bug 573210 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b2

People

(Reporter: sharparrow1, Assigned: sharparrow1)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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).
Attachment #452444 - Flags: review?(benjamin)
Comment on attachment 452444 [details] [diff] [review]
Patch

This is ugly. If necessary, introduce a self_type typedef.
Attachment #452444 - Flags: review?(benjamin) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
I was trying to match local style, but if you prefer something more like this...
Attachment #452444 - Attachment is obsolete: true
Attachment #452476 - Flags: review?(benjamin)
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.
Attachment #452476 - Flags: review?(benjamin) → review?(dbaron)
cc'ing my compiler experts
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?
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.
I wasn't really thinking about it; I'll change it to this->method().
Attached patch Patch v3Splinter Review
Updated to prefer "this->".
Attachment #452476 - Attachment is obsolete: true
Attachment #453300 - Flags: review?(dbaron)
Attachment #452476 - Flags: review?(dbaron)
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?
Attachment #453300 - Flags: review?(dbaron) → review+
(If you need/want someone to land it for you, you can add the 'checkin-needed' keyword.)
Keywords: checkin-needed
Assignee: nobody → sharparrow1
http://hg.mozilla.org/mozilla-central/rev/2c778979f15a
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3
Target Milestone: mozilla1.9.3 → mozilla1.9.3b2
You need to log in before you can comment on or make changes to this bug.