Closed
Bug 655865
Opened 13 years ago
Closed 13 years ago
Fix --enable-accessibility build with clang
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
VERIFIED
FIXED
mozilla6
People
(Reporter: espindola, Assigned: espindola)
Details
Attachments
(1 file, 1 obsolete file)
6.13 KB,
patch
|
jaas
:
review+
|
Details | Diff | Splinter Review |
Building with clang currently fails with accessibility is enabled.
Attachment #531183 -
Flags: review?
Comment 1•13 years ago
|
||
Comment on attachment 531183 [details] [diff] [review] Fix build with clang I don't see any problems with the patch, so I'd r+ it. But I think it makes sense to get opinion from objc guys. Steven, could you look please?
Attachment #531183 -
Flags: review? → review?(smichaud)
Comment 2•13 years ago
|
||
Comment on attachment 531183 [details] [diff] [review] Fix build with clang I've never used clang, and know very little about it. So I'm puzzled that you need to make these changes. But I can't say it's wrong to do so. In any case none of these changes should cause harm, so I'm happy to r+ this patch. One thing worth mentioning: 'Class' is defined (in exactly the same way) in /usr/include/objc/runtime.h, which you could include in nsAccessibleWrap.h using "#include <objc/runtime.h>" or "#import <objc/runtime.h>". Have you tried either of these? If they work, it might be better than defining Class yourself. Sounds like clang still has some bugs :-)
Attachment #531183 -
Flags: review?(smichaud) → review+
Comment 3•13 years ago
|
||
One more thing: You should probably add a comment saying what changes you've made, and why you made them (i.e. for compatibility with clang).
Comment 4•13 years ago
|
||
Finally, you probably should try to find someone who knows both Objective-C and clang. I can't understand why these would be the only changes needed to get clang to build the whole tree.
Assignee | ||
Comment 5•13 years ago
|
||
isEqualToString is defined in /System/Library/Frameworks/Foundation.framework/Versions/C/Headers/NSString.h as - (BOOL)isEqualToString:(NSString *)aString; I don't know obj-c++, but I am surprised that gcc is OK with passing a "const NSString *" to it.
Attachment #531183 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #531935 -
Flags: review?(joshmoz)
Attachment #531935 -
Flags: review?(joshmoz) → review+
Updated•13 years ago
|
Assignee: nobody → respindola
Keywords: checkin-needed
The author name in the patch seems to be corrupt. There's a character 0x81 after the Ã, and the last name looks like "EspÃndola" which doesn't seem right. I could probably fix it up, but some people are sensitive about such things :-).
Assignee | ||
Comment 7•13 years ago
|
||
ah, it is the traditional UTF-8 X iso8859-1 issue. I once had a conference badge with that :-) Any idea what is corrupting it? In any case, assuming bugzilla gets this box correct, by name is Rafael Ávila de Epíndola or with the acutes removed Rafael Avila de Espindola
Bugzilla does get it correct, but I already did my pushes for the day...
Updated•13 years ago
|
Status: NEW → ASSIGNED
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [fixed in cedar]
Version: unspecified → Trunk
Comment 9•13 years ago
|
||
(In reply to comment #8) > Bugzilla does get it correct Well, not entirely. Bugzilla uses UTF-8 for comments, but not for patches (see bug 633569)
Comment 10•13 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/dfb4cebe8c95
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → mozilla6
Comment 11•13 years ago
|
||
Build: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0 The fixes needed in order to build with clang are visible in the hg for the latest beta build (i.e. if ([attribute isEqualToString: (NSString*) kTopLevelUIElementAttribute]) in mozAccessible.mm). Setting this as Verified for Firefox 6 Beta.
Updated•13 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•