[PATCH] D145271: [MSVC compatibility][DLLEXPORT/DLLIMPORT] Allow dllexport/dllimport for local classes

[Wolfgang Pieb via Phabricator via llvm-commits]

wolfgangp added a comment.

In D145271#4171267 <https://reviews.llvm.org/D145271#4171267>, @hans wrote:

> Interesting! Do you have an example where this (local dllexport/import classes) comes up in practice?

A customer complained about the following code (I'm obscuring the class names) compiling with MSVC but
rejected by clang:

  template <class T>
  class __declspec(dllimport) A
  {
  };
  
  void func()
  {
    // local class with internal linkage
    class B: public A<B>
    {
    };
  }

The code derives from various instantiations of A in several contexts and it's causing extra work for the customer to do something different in a local context.
While I do agree that clang has it right I think this is more a case of bug-compatibility with MSVC.

> MSVC will also allow the following:
>
>   namespace {
>   struct __declspec(dllexport) T {
>       void foo();
>   };
>   }
>
> whereas Clang will not, and I think Clang is making the right call there.

Again, I do think clang makes the right call in all of these cases, but it's about MSVC compatibility, for better or worse.
I did also look at the anonymous namespace case above, but that's a bit more complicated, so I didn't address it this time.



================
Comment at: clang/lib/AST/Decl.cpp:492
+  // classes with dllexport/dllimport attributes.
+  if (spec->getASTContext().getTargetInfo().shouldDLLImportComdatSymbols())
+    if (spec->hasAttr<DLLExportAttr>() || spec->hasAttr<DLLImportAttr>())
----------------
hans wrote:
> We usually check ASTContext's `getTargetInfo().getCXXABI().isMicrosoft()`
That would not work for Playstation, since it's not following the Microsoft ABI. The call to shouldDLLImport...(), checks the triple (it includes Windows Itanium, Windows MSVC and PS).
Perhaps we need a less obscure predicate to indicate source compatibility with MSVC as opposed to MS ABI compatibility.


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

https://reviews.llvm.org/D145271