Page MenuHomePhabricator

Keep inherited dllimport/export attrs for explicit specialization of template class member functions
ClosedPublic

Authored by hans on Oct 4 2022, 6:08 AM.

Details

Summary

Previously we were stripping these normally inherited attributes during explicit specialization. However for class template member functions (but not function templates), MSVC keeps the attribute.

This makes Clang match that behavior, and fixes GitHub issue #54717

Diff Detail

Event Timeline

hans created this revision.Oct 4 2022, 6:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2022, 6:08 AM
Herald added a subscriber: mstorsjo. · View Herald Transcript
hans requested review of this revision.Oct 4 2022, 6:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2022, 6:08 AM
thakis accepted this revision.Oct 4 2022, 11:44 AM

lg

clang/lib/Sema/SemaDecl.cpp
7047

Here's hoping this doesn't break any actual code…

This revision is now accepted and ready to land.Oct 4 2022, 11:44 AM
hans added a comment.Oct 5 2022, 3:54 AM

This did turn up a problem.

With this patch Clang treats the following (based on https://source.chromium.org/chromium/chromium/src/+/main:media/formats/hls/source_string.h;l=125) as invalid, and so does MSVC:

template <typename> struct __declspec(dllexport) Foo {
    static Foo create();
};

// Don't allow creating Foo's of ints.
template <> Foo<int> Foo<int>::create() = delete; // MSVC: attempting to delete a __declspec(dllexport) function

The dllimport case errors similarly.

I'm tempted to make Clang keep allowing this. The intent of the code is perfectly clear -- the user is trying to delete the function, not export/import it -- and I can't think of any good way for the user to work around the error.

(Besides this issue, at least Chromium builds cleanly with this patch.)

hans added a comment.Oct 7 2022, 3:24 AM

Looking at the Chromium code some more, the case of trying to delete a specialization of a dllexport class template member seems suspect.

Since the error also matches MSVC I think we should keep that behavior, but I'll add a release note about it.