Last Comment Bug 655865 - Fix --enable-accessibility build with clang
: Fix --enable-accessibility build with clang
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-09 16:03 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-07-27 05:16 PDT (History)
5 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix build with clang (5.86 KB, patch)
2011-05-09 16:03 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
smichaud: review+
Details | Diff | Splinter Review
Patch using #include instead of typedef (6.13 KB, patch)
2011-05-12 08:05 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
jaas: review+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-05-09 16:03:12 PDT
Created attachment 531183 [details] [diff] [review]
Fix build with clang

Building with clang currently fails with accessibility is enabled.
Comment 1 alexander :surkov 2011-05-10 01:47:45 PDT
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?
Comment 2 Steven Michaud [:smichaud] (Retired) 2011-05-10 14:01:30 PDT
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 :-)
Comment 3 Steven Michaud [:smichaud] (Retired) 2011-05-10 14:03:54 PDT
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 Steven Michaud [:smichaud] (Retired) 2011-05-10 14:06:52 PDT
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.
Comment 5 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-05-12 08:05:28 PDT
Created attachment 531935 [details] [diff] [review]
Patch using #include instead of typedef

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.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-16 16:20:05 PDT
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 :-).
Comment 7 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-05-16 16:24:08 PDT
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
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-16 17:04:30 PDT
Bugzilla does get it correct, but I already did my pushes for the day...
Comment 9 :Ehsan Akhgari 2011-05-17 14:07:37 PDT
(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 Mounir Lamouri (:mounir) 2011-05-18 02:49:24 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/dfb4cebe8c95
Comment 11 AndreiD[QA] 2011-07-27 05:15:28 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.