[webkit-reviews] review denied: [Bug 58508] Fix linking with Sun Studio 12: function pointers for extern "C" are treated differently : [Attachment 89530] Fix linking issue on Solaris 10 with Sun Studio 12, Fix a linking problem for a plugin, as function pointers for extern "C" are handled different

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Ben Taylor
<bentaylor.solx86 at gmail.com>'s request for review:
Bug 58508: Fix linking with Sun Studio 12: function pointers for extern "C" are
treated differently
https://bugs.webkit.org/show_bug.cgi?id=58508

Attachment 89530: Fix linking issue on Solaris 10 with Sun Studio 12, Fix a
linking problem for a plugin,  as function pointers for extern "C" are handled
different
https://bugs.webkit.org/attachment.cgi?id=89530&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=89530&action=review

> Source/JavaScriptCore/wtf/MainThread.h:40
> +extern "C" {
> +    typedef void MainThreadFunction(void*);
> +}

This doesn't look good to me. We use callOnMainThread in many places, and every
time, we supply a C++ function to it (see e.g. Document::postTask()).

> Source/WebCore/ChangeLog:10
> +	   No new tests. (OOPS!)

The patch cannot be landed with this line. You can just remove it, since it's
obvious why there are no tests.

> Source/WebCore/plugins/npapi.cpp:179
> +extern "C" {
> +    void NPN_PluginThreadAsyncCall(NPP instance, void(*func) (void *), void
*userData)
> +    {
> +	   PluginMainThreadScheduler::scheduler().scheduleCall(instance, func,
userData);
> +    }
>  }

I don't understand why you had to wrap function body in extern "C". Its
declaration already has extern "C", so the compiler knows that it has C
linkage. Note that you didn't face a problem with NPN_ScheduleTimer, which also
takes a function pointer.

Would the following change alone do the trick?

    PluginMainThreadScheduler::scheduler().scheduleCall(instance,
(MainThreadFunction)func, userData);

And if that works, would a C++ style cast (probably a reinterpret_cast) work,
too?