MaskRay added a comment. In D133266#3776051 <https://reviews.llvm.org/D133266#3776051>, @bd1976llvm wrote:
> 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? First, does COFF use `-fvisibility-global-new-delete-hidden`? The impl of `-fvisibility-global-new-delete-hidden` is very different from -fvisibility. -fvisibility changed visibility is considered `!isVisibilityExplicit` while `-fvisibility-global-new-delete-hidden`'s is `isVisibilityExplicit`. dllspec and visibility are extensions for different object file formats and here we are mixing them together. Personally I don't favor defining too many "let A override B" designs. Adding `if (GV->hasDLLExportStorageClass() || GV->hasDLLImportStorageClass())` is mostly only due to compatibility consideration... >> 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
