[PATCH] D34992: Emit static constexpr member as available_externally definition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini marked 6 inline comments as done.
mehdi_amini added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2293
 /// If ExcludeCtor is true, the duration when the object's constructor runs
-/// will not be considered. The caller will need to verify that the object is
-/// not written to during its construction.
+/// will not be considered (unless trivial). The caller will need to verify that
+/// the object is not written to during its construction.
----------------
rsmith wrote:
> This comment change suggests that when `ExcludeCtor` is true, a trivial constructor is considered. That's the opposite of what this change does -- namely, a trivial constructor is not considered regardless of the value of `ExcludeCtor`.
Yep your right. I removed all the changes here.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2437-2445
+      bool HasMutableField = false;
+      if (Context.getLangOpts().CPlusPlus) {
+        if (const CXXRecordDecl *Record =
+                Context.getBaseElementType(D->getType())->getAsCXXRecordDecl())
+          HasMutableField = Record->hasMutableFields();
+      }
+
----------------
rsmith wrote:
> This duplicates much of the work done by `isTypeConstant`.
Indeed, thanks for pointing this, this simplifies a lot!


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2448-2451
+        if (InitExpr) {
+          GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+          GV->setInitializer(EmitConstantInit(*InitDecl));
+        }
----------------
rsmith wrote:
> In order for this transformation to be correct, you need to know that the variable has static initialization, which means that it needs to formally have a constant initializer. You can use `D->isInitKnownICE()` to check this.
Done! But couldn't find how to express it as a test-case though.


https://reviews.llvm.org/D34992