Future compilers/runtimes will warn if we try to use objc_msgSend without the correct casts. We should add casts in WebKit/WebCore when we call objc_msgSend or IMP.
<rdar://problem/10906849>
Created attachment 161060 [details] Patch
Comment on attachment 161060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161060&action=review > Source/WebKit/mac/WebView/WebDelegateImplementationCaching.mm:87 > - return objc_msgSend(delegate, selector, self); > + return reinterpret_cast<id (*)(id, SEL, id)>(objc_msgSend)(delegate, selector, self); What about making a bunch of webkit_objc_msgSend like this?: template<typename Arg1Type, typename Arg2Type> id webkit_objc_msgSend(id target, SEL selector, Arg1Type arg1, Arg2Type arg2) { return reinterpret_cast<id (*)(id, SEL, Arg1Type, Arg2Type)>(objc_msgSend)(target, selector, arg1, arg2); }
Maybe we should also modify the style checker to prevent bare use of objc_msgSend?
Comment on attachment 161060 [details] Patch Clearing the review flag, Pratik has an update.
Created attachment 161264 [details] Patch
(In reply to comment #3) > (From update of attachment 161060 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161060&action=review > > > Source/WebKit/mac/WebView/WebDelegateImplementationCaching.mm:87 > > - return objc_msgSend(delegate, selector, self); > > + return reinterpret_cast<id (*)(id, SEL, id)>(objc_msgSend)(delegate, selector, self); > > What about making a bunch of webkit_objc_msgSend like this?: > template<typename Arg1Type, typename Arg2Type> > id webkit_objc_msgSend(id target, SEL selector, Arg1Type arg1, Arg2Type arg2) > { > return reinterpret_cast<id (*)(id, SEL, Arg1Type, Arg2Type)>(objc_msgSend)(target, selector, arg1, arg2); > } This was a great idea. New patch uploaded with this template code.
Comment on attachment 161264 [details] Patch r=me
Comment on attachment 161264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161264&action=review > Source/WebKit/mac/WebView/WebDelegateImplementationCaching.mm:86 > > +template<typename Arg1Type> > +id webkit_objc_msgSend(id target, SEL selector, Arg1Type arg1) > +{ > + return reinterpret_cast<id (*)(id, SEL, Arg1Type)>(objc_msgSend)(target, selector, arg1); > +} I would put the templates in a new header in WTF. Then I would ban the use of objc_msgSend through the style checker. I am afraid new code with objc_msgSend might come without the reviewer knowing about this change.
This template is limited in what it can do - specifically, it assumes that the functions returns an id.
Comment on attachment 161264 [details] Patch Attachment 161264 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13686455 New failing tests: CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread
How did this fail? Searching for CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread on the log linked to above, I see [----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate [ RUN ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread [ OK ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread (120 ms) [----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate (120 ms total) What failed?
(In reply to comment #12) > How did this fail? Searching for CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread on the log linked to above, I see > > [----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate > [ RUN ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread > [ OK ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread (120 ms) > [----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate (120 ms total) > > What failed? It is likely a sick bot. You patch does not touch anything on Linux.
Created attachment 161524 [details] Patch
Comment on attachment 161524 [details] Patch Very nice!
Committed r127193: <http://trac.webkit.org/changeset/127193>
Looks like llvm-gcc doesn't like my patch. From <http://webkit-commit-queue.appspot.com/results/13724238> In file included from /Volumes/Data/WebKit/Source/WebCore/page/mac/EventHandlerMac.mm:53: /Volumes/Data/WebKit/WebKitBuild/Release/usr/local/include/wtf/ObjcRuntimeExtras.h:29: error: default template arguments may not be used in function templates /Volumes/Data/WebKit/WebKitBuild/Release/usr/local/include/wtf/ObjcRuntimeExtras.h:35: error: no default argument for 'Arg1Type' /Volumes/Data/WebKit/WebKitBuild/Release/usr/local/include/wtf/ObjcRuntimeExtras.h:35: error: default template arguments may not be used in function templates .... and more such errors like this. I'm looking into it.
Ok. I can reproduce the build failure by switching to llvm-gcc-4.2 on Mountain Lion (and removing a few -W arguments to the compiler). The template format is template<typename RetType = id, typename Arg1Type> RetType wtfObjcMsgSend(id target, SEL selector, Arg1Type arg1) { return reinterpret_cast<RetType (*)(id, SEL, Arg1Type)>(objc_msgSend)(target, selector, arg1); } If I remove the " = id" part, it's happy. I could do that. And then have each call site explicitly specify the return type. Alternatively, what are the chances we can get the EWS bot updated with a more recent toolchain?
> If I remove the " = id" part, it's happy. I could do that. And then have each call site explicitly specify the return type. Alternatively, what are the chances we can get the EWS bot updated with a more recent toolchain? I am afraid we have to support the old compilers. If the EWS have an old SDK, chances are many people do too. I think the best is add an explanation in a FIXME and remove the default arg :( :(
(In reply to comment #19) > > If I remove the " = id" part, it's happy. I could do that. And then have each call site explicitly specify the return type. Alternatively, what are the chances we can get the EWS bot updated with a more recent toolchain? > > I am afraid we have to support the old compilers. > If the EWS have an old SDK, chances are many people do too. > > I think the best is add an explanation in a FIXME and remove the default arg :( :( I agree with Benjamin. Please either do that soon or roll out http://trac.webkit.org/changeset/127193 (it is causing the mac-ews queue to get clogged).
Working on this now. I should have a patch soon.
Reopening to attach new patch.
Created attachment 161752 [details] Fix for llvm-gcc
Comment on attachment 161752 [details] Fix for llvm-gcc Looks good. I think we should land this so we can take the EWS back online for everyone. Then we fix the EWS and we can eventually revert the patch.
Committed r127306: <http://trac.webkit.org/changeset/127306>
Created attachment 161762 [details] Patch
Comment on attachment 161762 [details] Patch Damn GCC.
Committed r127313: <http://trac.webkit.org/changeset/127313>