Bug 191201 - Do not re-declare soft-linked functions in SOFT_LINK* macros
Summary: Do not re-declare soft-linked functions in SOFT_LINK* macros
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-11-02 11:39 PDT by Jer Noble
Modified: 2021-11-01 12:47 PDT (History)
3 users (show)

See Also:


Attachments
Patch (38.79 KB, patch)
2018-11-02 11:44 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (38.89 KB, patch)
2018-11-02 11:52 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (38.97 KB, patch)
2018-11-02 11:58 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2018-11-02 11:39:46 PDT
Do not re-declare soft-linked functions in SOFT_LINK* macros
Comment 1 Jer Noble 2018-11-02 11:44:39 PDT
Created attachment 353718 [details]
Patch
Comment 2 Jer Noble 2018-11-02 11:52:58 PDT
Created attachment 353719 [details]
Patch
Comment 3 Jer Noble 2018-11-02 11:58:21 PDT
Created attachment 353721 [details]
Patch
Comment 4 Alexey Proskuryakov 2018-11-02 12:31:40 PDT
Comment on attachment 353721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353721&action=review

> Source/WTF/ChangeLog:8
> +        Re-declaring soft-linked functions can cause build errors when upstream frameworks change, for example, their export attributes

This is a good thing, we don’t want to diverge from what the framework declares. 

It’s similar to how we redeclare as much as possible in SOI headers.
Comment 5 Jer Noble 2018-11-02 12:48:31 PDT
Comment on attachment 353721 [details]
Patch

(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 353721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353721&action=review
> 
> > Source/WTF/ChangeLog:8
> > +        Re-declaring soft-linked functions can cause build errors when upstream frameworks change, for example, their export attributes
> 
> This is a good thing, we don’t want to diverge from what the framework
> declares. 
> 
> It’s similar to how we redeclare as much as possible in SOI headers.

I get the goal. But we explicitly redeclare them as __always_inline__ and visibility('hidden') attributes to our own redeclaration, so we are explicitly redeclaring them to be different than the framework.  So if this is our goal, we need to stop doing that. Until then, your premise is invalid.
Comment 6 Jer Noble 2018-11-02 12:51:33 PDT
(In reply to Jer Noble from comment #5)
> Comment on attachment 353721 [details]
> Patch
> 
> (In reply to Alexey Proskuryakov from comment #4)
> > Comment on attachment 353721 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=353721&action=review
> > 
> > > Source/WTF/ChangeLog:8
> > > +        Re-declaring soft-linked functions can cause build errors when upstream frameworks change, for example, their export attributes
> > 
> > This is a good thing, we don’t want to diverge from what the framework
> > declares. 
> > 
> > It’s similar to how we redeclare as much as possible in SOI headers.
> 
> I get the goal. But we explicitly redeclare them as __always_inline__ and
> visibility('hidden') attributes to our own redeclaration, so we are
> explicitly redeclaring them to be different than the framework.  So if this
> is our goal, we need to stop doing that. Until then, your premise is invalid.

Also, we do /not/ match the declarations in the SPI headers either, since we routinely have to add "NS_ASSUME_NONNULL_BEGIN/END" macros around classes and methods.
Comment 7 Alex Christensen 2021-11-01 12:47:25 PDT
Comment on attachment 353721 [details]
Patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.