[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13896
+          Diag(var->getLocation(), diag::err_constexpr_var_requires_const_init)
+              << var << Init->getSourceRange();
+      }
----------------
efriedma wrote:
> zahiraam wrote:
> > efriedma wrote:
> > > zahiraam wrote:
> > > > efriedma wrote:
> > > > > I don't understand why this diagnostic is necessary.
> > > > Now that the DLLImport variable can be a constant, HasConstInit is returning true (it was before returning false) and when the variable is global a diagnostic should be reported. 
> > > > 
> > > > This:
> > > >   extern int _declspec(dllimport) val;
> > > >   constexpr int& val_ref = val;
> > > > 
> > > > should report a diagnostic, but this:
> > > > 
> > > >   int foo() { 
> > > >      extern int _declspec(dllimport) val; 
> > > >      constexpr int& val_ref = val;
> > > >   }
> > > > 
> > > >  shouldn't report a diagnostic.
> > > MSVC doesn't report a diagnostic for either of your examples?
> > So, these shouldn't fail anymore? 
> > https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/PR19955.cpp#L5
> > https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/PR19955.cpp#L8
> > https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/dllimport-constexpr.cpp#L44
> Right, they shouldn't fail.
> 
> My perspective is that since we're able to give those the expected semantics, we should just do that.  The only reason that "dllimport" is relevant at all is that there isn't a relocation for it, but our plan is to cover that up with a runtime constructor. So the user shouldn't need to know there's trickery behind the scenes unless they disassemble the binary.
Got it! Thanks. Will remove the diagnostic here then. 


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

https://reviews.llvm.org/D137107