MaskRay added a comment. In D133266#3775384 <https://reviews.llvm.org/D133266#3775384>, @MaskRay wrote:
> In D133266#3775189 <https://reviews.llvm.org/D133266#3775189>, @bd1976llvm > wrote: > >> This approach doesn't account for the implementation of >> -fvisibility-global-new-delete-hidden. The following case now fails after >> this change: >> >> >type new_del.cpp >> >> namespace std { >> typedef __typeof__(sizeof(int)) size_t; >> struct nothrow_t {}; >> struct align_val_t {}; >> } >> __declspec(dllexport) void operator delete(void* ptr) throw() {return;} >> >> >> >clang.exe --target=x86_64-pc-windows-gnu new_del.cpp >> -fvisibility-global-new-delete-hidden -c >> new_del.cpp:8:28: error: non-default visibility cannot be applied to >> 'dllexport' declaration >> __declspec(dllexport) void operator delete(void* ptr) throw() {return;} >> ^ >> 1 error generated. > > 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 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`? if (LV.isVisibilityExplicit()) { // Reject explicit hidden visibility on dllexport and hidden/protected // visibility on dllimport. if (GV->hasDLLExportStorageClass() && LV.getVisibility() == HiddenVisibility) getDiags().Report(D->getLocation(), diag::err_hidden_visibility_dllexport); if (GV->hasDLLImportStorageClass() && LV.getVisibility() != DefaultVisibility) getDiags().Report(D->getLocation(), diag::err_non_default_visibility_dllimport); } 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
