[PATCH] D133266: [MinGW] Reject explicit non-default visibility applied to dllexport/dllimport declaration

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

In D133266#3775832 <https://reviews.llvm.org/D133266#3775832>, @MaskRay wrote:

> In D133266#3775384 <https://reviews.llvm.org/D133266#3775384>, @MaskRay wrote:
>
>> In D133266#3775189 <https://reviews.llvm.org/D133266#3775189>, @bd1976llvm wrote:
>>
>>> 
>>
>> From a glance, `-fvisibility-global-new-delete-hidden` should make the visibility implicit (so won't trigger this error) but don't seem to do so? I'll investigate later.
>
> `-fvisibility-global-new-delete-hidden` is implemented by adding `VisibilityAttr` to declarations.
> I think  `-fvisibility-global-new-delete-hidden` triggering the error is fine. The alternative, adding a rule that `__declspec(dllexport) void operator delete` does not get hidden visibility, seems ad-hoc to me.

I'm not sure why this visibility option (`-fvisibility-global-new-delete-hidden`) is implemented differently to the others (e.g. `-fvisibility=hidden`)? `__declspec(dllexport) void operator delete` does not get hidden visibility, might be difficult to implement; but OTOH, the dllstorageclass overrides the visibility set by a visibility option for the other visibility options (e.g. -fvisibility=hidden) and it would be nice to have consistent behaviour for all the visibility options. Would be great to get other peoples opinions on whether an error here would be a problem?

> I think the only needed change is to allow dllexport protected, but then we probably need two diagnostics. Perhaps `diag::err_hidden_visiblity_dllexport` and `diag::err_non_default_visibility_dllimport`?

SGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133266/new/

https://reviews.llvm.org/D133266