Bug 193815 - Move soft-linking of Lookup.framework out of LookupSPI.h
Summary: Move soft-linking of Lookup.framework out of LookupSPI.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks: 194529
  Show dependency treegraph
 
Reported: 2019-01-24 22:32 PST by David Kilzer (:ddkilzer)
Modified: 2019-02-11 21:08 PST (History)
4 users (show)

See Also:


Attachments
Patch v1 (28.13 KB, patch)
2019-01-24 22:38 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (33.42 KB, patch)
2019-01-25 10:48 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2019-01-24 22:32:10 PST
Move soft-linking of Lookup.framework out of LookupSPI.h.

Header should never soft-link frameworks or libraries since that code can be included in multiple translation units (source files), causing duplicate code to be generated.  See Bug 193750 for a new check-webkit-style check that flags this issue.
Comment 1 David Kilzer (:ddkilzer) 2019-01-24 22:38:28 PST
Created attachment 360083 [details]
Patch v1
Comment 2 Radar WebKit Bug Importer 2019-01-24 22:39:03 PST
<rdar://problem/47540816>
Comment 3 Tim Horton 2019-01-24 23:54:03 PST
Comment on attachment 360083 [details]
Patch v1

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

> Source/WTF/wtf/cocoa/SoftLinking.h:632
> +#define SOFT_LINK_VARIABLE_MAY_FAIL_FOR_HEADER(functionNamespace, framework, variableName, variableType) \

'Variable'?! Why is this different from SOFT_LINK_CONSTANT_*? (I'm too lazy to compare)
Comment 4 David Kilzer (:ddkilzer) 2019-01-25 10:15:37 PST
(In reply to Tim Horton from comment #3)
> Comment on attachment 360083 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360083&action=review
> 
> > Source/WTF/wtf/cocoa/SoftLinking.h:632
> > +#define SOFT_LINK_VARIABLE_MAY_FAIL_FOR_HEADER(functionNamespace, framework, variableName, variableType) \
> 
> 'Variable'?! Why is this different from SOFT_LINK_CONSTANT_*? (I'm too lazy
> to compare)

I made a bad design decision when implementing the SOFT_LINK_CONSTANT*() macros, which was to include a local declaration of the constant like this:

#define SOFT_LINK_CONSTANT_FOR_HEADER(functionNamespace, framework, variableName, variableType) \
    WTF_EXTERN_C_BEGIN \
    extern const variableType variableName; \
    WTF_EXTERN_C_END \
    namespace functionNamespace { \
    variableType get_##framework##_##variableName(); \
    }

If that declaration exactly doesn't match the original (either in an *SPI.h header or the "real" header), then we get compiler warnings-as-errors.

Furthermore, the actual soft-linking code doesn't care about `const` keywords in the `variableType`, so to keep the declaration and keep the macros working, we'd need two different types (one that includes the original `const` keywords and one that doesn't).

The simple fix is to separate concerns and let the declaration reside in an *SPI.h header or the "real" header, and remove the declaration from the SOFT_LINK_CONSTANT*() macros.  (I tried it and the macOS Release build didn't break, so I'll make that change along with this patch.)
Comment 5 David Kilzer (:ddkilzer) 2019-01-25 10:48:32 PST
Created attachment 360124 [details]
Patch v2
Comment 6 WebKit Commit Bot 2019-01-25 12:24:19 PST
Comment on attachment 360124 [details]
Patch v2

Clearing flags on attachment: 360124

Committed r240494: <https://trac.webkit.org/changeset/240494>
Comment 7 WebKit Commit Bot 2019-01-25 12:24:21 PST
All reviewed patches have been landed.  Closing bug.