Bug 207380 - check-webkit-style: Check if *_EXPORT and *_EXPORT_PRIVATE macros are used under corresponding directories
Summary: check-webkit-style: Check if *_EXPORT and *_EXPORT_PRIVATE macros are used un...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-07 03:58 PST by Fujii Hironori
Modified: 2020-02-20 19:06 PST (History)
9 users (show)

See Also:


Attachments
Patch (6.48 KB, patch)
2020-02-07 04:16 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (6.75 KB, patch)
2020-02-09 20:14 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Existing build/export_macro errors (5.98 KB, text/plain)
2020-02-09 20:15 PST, Fujii Hironori
no flags Details
Patch (6.80 KB, patch)
2020-02-13 03:22 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (6.63 KB, patch)
2020-02-16 19:10 PST, Fujii Hironori
jbedard: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2020-02-07 03:58:06 PST
check-webkit-style: Check if *_EXPORT and *_EXPORT_PRIVATE macros are used under corresponding directories
Comment 1 Fujii Hironori 2020-02-07 04:16:42 PST
Created attachment 390073 [details]
Patch
Comment 2 Fujii Hironori 2020-02-09 20:11:56 PST
Comment on attachment 390073 [details]
Patch

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

> Tools/Scripts/webkitpy/style/checkers/cpp.py:152
> +    'WTF_EXPORT': None,

None doesn't work as I expected. WTF_EXPORT shouldn't appear in any directories.
Comment 3 Fujii Hironori 2020-02-09 20:14:01 PST
Created attachment 390225 [details]
Patch
Comment 4 Fujii Hironori 2020-02-09 20:15:17 PST
Created attachment 390226 [details]
Existing build/export_macro errors

python .\Tools\Scripts\check-webkit-style --filter=-,+build/export_macro .\Source .\Tools

I'm going to fix them in another bug ticket.
Comment 5 Darin Adler 2020-02-12 15:54:38 PST
Comment on attachment 390073 [details]
Patch

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

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:152
>> +    'WTF_EXPORT': None,
> 
> None doesn't work as I expected. WTF_EXPORT shouldn't appear in any directories.

Are you saying we should never use WTF_EXPORT for anything at all? I don’t understand.
Comment 6 Darin Adler 2020-02-12 15:56:13 PST
I want to say review+ on this. I really appreciate you adding this!

I don’t understand the state of the 40 existing errors. Are they all mistakes?

It would be nice to fix them either before landing this checker or as part of the same patch. Unless that’s risky?
Comment 7 Don Olmstead 2020-02-12 16:44:41 PST
The only place that WTF_IMPORT and WTF_EXPORT should be referenced directly is when declaring a macro for exports for a library. Exports within WTF should be marked using WTF_EXPORT_PRIVATE.

Three appeared to have been a handful of places where WTF_EXPORT was used directly instead of WTF_EXPORT_PRIVATE. He fixed those over in https://trac.webkit.org/changeset/256420/webkit so I think this is good now.

For ports using visibility attributes there isn't a problem since WTF_EXPORT and WTF_IMPORT expand to the same thing. But for ports using declspec it will always expand to __declspec(dllexport) and will never expand to __declspec(dllimport).

It would possibly be better to rename WTF_EXPORT and WTF_IMPORT to something else because its an honest mistake to use WTF_EXPORT instead of WTF_EXPORT_PRIVATE, but with this style check working we shouldn't hit this issue going forward.
Comment 8 Fujii Hironori 2020-02-12 17:58:11 PST
JSC is using both JS_EXPORT and JS_EXPORT_PRIVATE. JS_EXPORT is for public API. JS_EXPORT_PRIVATE for private API.
However, their macro definitions are same for All platfrom. They can be used interchangeably.
I don't know the reason to have both.

WTF has WTF_EXPORT and WTF_EXPORT_PRIVATE
WTF was using WTF_EXPORT and WTF_EXPORT_PRIVATE for function declarations.
But, WTF_EXPORT definition was useless for declarations for Windows port because it is always __declspec(dllexport).
So, I replaced all WTF_EXPORT with WTF_EXPORT_PRIVATE in declarations.
Comment 9 Fujii Hironori 2020-02-13 00:15:50 PST
Comment on attachment 390225 [details]
Patch

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

> Tools/Scripts/webkitpy/style/checkers/cpp.py:147
> +    'JS_EXPORT_PRIVATE': 'Source/JavaScriptCore',

Add JS_EXPORT.
Comment 10 Fujii Hironori 2020-02-13 03:22:07 PST
Created attachment 390627 [details]
Patch
Comment 11 Fujii Hironori 2020-02-16 19:03:36 PST
Comment on attachment 390627 [details]
Patch

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

> Tools/Scripts/webkitpy/style/checkers/cpp.py:154
> +    'WTF_EXPORT': None,

WTF_EXPORT was removed in r256720.
Comment 12 Fujii Hironori 2020-02-16 19:10:06 PST
Created attachment 390896 [details]
Patch
Comment 13 Fujii Hironori 2020-02-16 20:17:12 PST
Windows EWS failed to compile.

> Could NOT find interface definition for SVGGradientElement in ./svg/SVGGradientElement.idl at /home/buildbot/worker/Windows-EWS/build/Source/WebCore/bindings/scripts/CodeGenerator.pm line 502.

It seems Bug 206565.
Comment 14 Fujii Hironori 2020-02-20 19:05:25 PST
Committed r257124: <https://trac.webkit.org/changeset/257124>
Comment 15 Radar WebKit Bug Importer 2020-02-20 19:06:14 PST
<rdar://problem/59655194>