[PATCH] D45898: [SemaCXX] Mark destructor as referenced

[John McCall via Phabricator via cfe-commits]

rjmccall added a reviewer: doug.gregor.
rjmccall added inline comments.


================
Comment at: lib/Sema/SemaInit.cpp:7074
+          if (RD->hasDefinition() && RD->hasNonTrivialDestructor())
+            S.MarkFunctionReferenced(E->getExprLoc(), S.LookupDestructor(RD));
+
----------------
If we're actually using the destructor, we can't just look it up and mark it referenced; we have to call CheckDestructorAccess and DiagnoseUseOfDecl.

Walking the syntactic initializers like this seems wrong:
  - I don't know if we actually promise to rewrite these expressions to the actual recursive element initialization expression if the expression requires 
conversion.
  - This is going to miss implicitly-initialized elements, which can happen for a variety of reasons including (1) running out of explicit initializers and (2) designated initializers skipping a field.
  - We'll end up with redundant diagnostics from CheckDestructorAccess/DiagnoseUseOfDecl if the expression is an already-materialized temporary.
  - We'll end up with unwanted diagnostics from CheckDestructorAccess/DiagnoseUseOfDecl if the expression is a gl-value and actually part of a reference-initialization, where we don't actually use the destructor.

I think the right solution is probably that list-initialization needs to recognize that the initialization of a record needs to be treated as a potential use of the destructor even if we aren't supposed to bind it as a temporary, because once an object is initialized in the local context, there are all sorts of reasons why we might need to call its destructor before the initialization completes/transitions.  Basically every call to shouldBindAsTemporary in this file is suspect, because they're all places where we might need to check the destructor use even if we didn't make a temporary.

We seem to get this right in SK_UserConversion where we call shouldDestroyEntity, and the comment there is spot-on — it doesn't make sense for this code to be specific to SK_UserConversion at all.  My tentative suggestion would be to (1) make a helper function that does that exact "if should-bind then bind else if should-destroy then destroy" dance and then (2) look at all the should-bind checks and try to use your helper function instead.  But I'd really like Richard and/or Doug to weigh in.


================
Comment at: test/CodeGenObjCXX/arc-list-init-destruct.mm:1
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.13.0 -std=c++1z -fobjc-arc -fobjc-exceptions -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s
+
----------------
Does the corresponding C++ test case (replacing `Class0 *f;` with `HasExplicitNonTrivialDestructor f;`) not reproduce the problem?


Repository:
  rC Clang

https://reviews.llvm.org/D45898