[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

[Takuto Ikuta via Phabricator via cfe-commits]

takuto.ikuta marked 4 inline comments as done.
takuto.ikuta added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.def:117
 LANGOPT(Digraphs          , 1, 0, "digraphs")
+LANGOPT(DllexportInlines  , 1, 1, "If dllexport a class should dllexport implicit inline methods in the Microsoft ABI")
 BENIGN_LANGOPT(HexFloats  , 1, C99, "C99 hexadecimal float constants")
----------------
hans wrote:
> Same comment about "implicit" here as above.
> 
> And is the "In the MS ABI" part of the comment important? Does the flag change depending on ABI?
> 
> 
> And is the "In the MS ABI" part of the comment important? Does the flag change depending on ABI?

Currently it works only for Microsoft ABI.
In this patch, I want to focus only on Microsoft ABI.


================
Comment at: clang/include/clang/Basic/LangOptions.h:217
 
+  /// If set, dllexported classes dllexport their implicit inline methods.
+  bool DllexportInlines = true;
----------------
hans wrote:
> not sure what you mean by implicit here.. this applies to all inline defined member functions.
Dropped.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5542
+    // or checkClassLevelDLLAttribute?
+    if (MD->isInlined() && getDLLAttr(Member)) {
+      const Stmt* Body = nullptr;
----------------
hans wrote:
> Hmm, yes I think the intention was to not put the attribute on the members so I'm not sure why this is needed?
> Hmm, yes I think the intention was to not put the attribute on the members so I'm not sure why this is needed?

I changed the code around here from dropping DLL attribute to warning static local variable.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5556
+          Body && !Checker.Visit(Body)) {
+        MD->dropAttr<DLLExportAttr>();
+      }
----------------
takuto.ikuta wrote:
> I noticed that this does not work correctly yet.
> Occasionally, some functions are not inlined but not exported, and those cannot be linked.
I gave up to support local static variable.


https://reviews.llvm.org/D51340