Closed Bug 573210 Opened 10 years ago Closed 10 years ago

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


(Core :: XPCOM, defect)

Not set





(Reporter: sharparrow1, Assigned: sharparrow1)



(1 file, 2 obsolete files)

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

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


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