[PATCH] D19275: Do not register incompatible C++ destructors with __cxa_atexit

[Derek Schuff via cfe-commits]

dschuff added inline comments.

================
Comment at: lib/CodeGen/CGDeclCXX.cpp:92
@@ +91,3 @@
+  // disabled via a flag, a different helper function is generated anyway.
+  const CXXRecordDecl *Record = type->getAsCXXRecordDecl();
+  bool CanRegisterDestructor = Record &&
----------------
jfb wrote:
> Can you do that unconditionally? Or do you have to test for `dtorKind == QualType::DK_cxx_destructor` first? It looks like the function already does a `dyncast_or_null` so the previous code was doing a check it didn't need to.
Yes, that's why I made it unconditional. And actually I just noticed that the switch statement above returns from the function if `dtorKind != DK_cxx_destructor` anyway, so I removed the check here too.

================
Comment at: lib/CodeGen/CGDeclCXX.cpp:95
@@ +94,3 @@
+                               !CGM.getCXXABI().HasThisReturn(GlobalDecl(
+                                   Record->getDestructor(), Dtor_Complete));
+
----------------
jfb wrote:
> Comment on why you can't register dtor when has this return (ABI mismatch).
Doesn't the new text in the comment right above on line 98 cover that?

================
Comment at: lib/CodeGen/CGDeclCXX.cpp:97
@@ +96,3 @@
+
+  if (dtorKind == QualType::DK_cxx_destructor && Record &&
+      (CanRegisterDestructor || !CGM.getCodeGenOpts().CXAAtExit)) {
----------------
jfb wrote:
> Do you need to check `Record` here? Doesn't it also imply `dtorKind == QualType::DK_cxx_destructor`?
Yes, there can be a case where `Record` is nullptr and `CGM.getCodeGenOpts().CXAAtExit` is false.

================
Comment at: lib/CodeGen/CGDeclCXX.cpp:98
@@ +97,3 @@
+  if (dtorKind == QualType::DK_cxx_destructor && Record &&
+      (CanRegisterDestructor || !CGM.getCodeGenOpts().CXAAtExit)) {
+    assert(!Record->hasTrivialDestructor());
----------------
jfb wrote:
> ` || !CGM.getCodeGenOpts().CXAAtExit` seems wrong here. Shouldn't the condition be `CanRegisterDestructor && CGM.getCodeGenOpts().CXAAtExit`? I may be misunderstanding...
I split and expanded the comment and made a variable with a name. See if it makes more sense now.


http://reviews.llvm.org/D19275