[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/CodeGen/CGDeclCXX.cpp:572
     PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn));
+  } else if (D->hasConstantInitialization() && !(D->hasAttr<ConstInitAttr>())) {
+    OrderGlobalInitsOrStermFinalizers Key(201,
----------------
efriedma wrote:
> zahiraam wrote:
> > efriedma wrote:
> > > zahiraam wrote:
> > > > efriedma wrote:
> > > > > zahiraam wrote:
> > > > > > efriedma wrote:
> > > > > > > How is ConstInitAttr relevant here?
> > > > > > This change made (without the !(D->hasAttr<ConstInitAttr>()) made the LIT behavior of aix-static-init.cpp. The IR generated for 
> > > > > > namespace test3 {
> > > > > >   struct Test3 {
> > > > > >     constexpr Test3() {};
> > > > > >     ~Test3() {};
> > > > > >   };
> > > > > > 
> > > > > >   constinit Test3 t;
> > > > > > } // namespace test3
> > > > > > 
> > > > > > was different. I would have thought that the change we made for constexpr wouldn't affter constinit? 
> > > > > I think the significant bit there isn't the use of constinit; it's the non-trivial destructor.  I think the priority modification should only affect constructors, not destructors.  (Not sure how to make that work, at first glance.)
> > > > Let's see if this is an acceptable solution.
> > > To fake constant initialization, we need to initialize the variable before anything else runs.  But the rearranged prioritization isn't supposed to affect the destructor.  From [basic.start.term]: "If an object is initialized statically, the object is destroyed in the same order as if the object was dynamically initialized."
> > > 
> > > What you're doing here isn't exactly implementing that.  What you're doing here is delaying both the initialization and the destruction if the variable has a non-trivial destructor.  We need to separate the two to get the behavior we want.
> > Could we consider adding a field to EvaluatedStmt called "HasTrivialDestrutor" and only perform the prioritization change when 
> > !D->HasTrivialDesctructor?  Instead of using the test for D->hasConstantInitialization(). This seems to be englobing to many cases.
> > 
> > I considered returning null for HasConstantInitialization in case of var has a non-trivial destructor but that doesn't seem to work.
> Once you separate out the initialization from the destruction, the rest should come together naturally, I think? I'm not sure what other cases could be caught by hasConstantInitialization().
Does this change accomplish this? Thanks.


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

https://reviews.llvm.org/D137107