Bug 207380 – check-webkit-style: Check if *_EXPORT and *_EXPORT_PRIVATE macros are used under corresponding directories

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Fujii Hironori]

check-webkit-style: Check if *_EXPORT and *_EXPORT_PRIVATE macros are used under corresponding directories

[Fujii Hironori]

Created attachment 390073 [details]
Patch

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 390225 [details]
Patch

[Fujii Hironori]

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.

[Darin Adler]

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.

[Darin Adler]

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?

[Don Olmstead]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 390627 [details]
Patch

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 390896 [details]
Patch

[Fujii Hironori]

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.

[Fujii Hironori]

Committed r257124: <https://trac.webkit.org/changeset/257124>

[Radar WebKit Bug Importer]

<rdar://problem/59655194>