bd1976llvm added a comment. In D133266#3776164 <https://reviews.llvm.org/D133266#3776164>, @MaskRay wrote:
> 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... Sorry for the slow reply. -fvisibility-global-new-delete-hidden was implemented here: https://reviews.llvm.org/D53787, it is Clang only (it is not in GCC) and, as you pointed out, the changed visibility is deliberately `isVisibilityExplicit`. We are happy to accept the change in behaviour as we don't think it will impact customers. >>> 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! LGTM. Thanks for your patience. ================ Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1116 + if (GV->hasDLLExportStorageClass()) { + // Reject explicit hidden visibility on dllexport. + if (LV.getVisibility() == HiddenVisibility) ---------------- Opinion: I don't think these comments are needed as the code is clear. I think it would be better to have a single comment at the top of this block e.g. "Reject incompatible dlllstorageclass and visibility annotations" or just remove them. 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
