[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D145369#4181296 <https://reviews.llvm.org/D145369#4181296>, @ilya-biryukov wrote:

> I don't have a lot of experience in codegen, so will let Aaron and Richard do the review.

Thanks for checking! I don't have a lot of experience here either, so any review is much appreciated.

> However, still wanted to share one observation. The actual check that avoids emitting the destructors for variables seems more involved than just checking if the destructor is `constexpr`.
> Is it safe to rely solely on the type for codegen? E.g. I would have expected checks that look at `HasConstantDestruction` for variables with non-trivial constructors.

Right, codegen is also factoring in the check about emitting destructors when deciding about marking the LLVM value constant or not:

  in CodeGenModule::EmitGlobalVarDefinition()
  
    bool NeedsGlobalDtor =
        !IsDefinitionAvailableExternally &&
        D->needsDestruction(getContext()) == QualType::DK_cxx_destructor;
  
  ...
  
    // If it is safe to mark the global 'constant', do so now.
    GV->setConstant(!NeedsGlobalCtor && !NeedsGlobalDtor &&
                    isTypeConstant(D->getType(), true));

So I *think* this is correct, and that the `hasConstexprDestructor()` case was just missing from `isTypeConstant()` because constexpr destructors were added later. But again, not super familiar with this stuff :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145369