[PATCH] D154581: [clang][Interp] Track existing InitMaps in InterpState

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.


================
Comment at: clang/lib/AST/Interp/Descriptor.cpp:42
                         const Descriptor *D) {
+  new (Ptr) InitMapPtr(std::nullopt);
+
----------------
aaron.ballman wrote:
> This worries me a little bit for a few reasons, but it might be okay:
> 
> * What validates that the bytes pointed to by `Ptr` are aligned properly for an `InitMapPtr` object? Perhaps we need an `alignas` in the function signature to ensure correct alignment of those bytes?
> * `InitMapPtr` is `std::optional<std::pair<bool, std::shared_ptr<InitMap>>>` and I *think* using placement new will ensure we have correct objects in all the expected places, but I'm not 100% sure because we're calling the `nullopt` constructor here.
> * I *think* it is correct that you are not assigning the result of the placement `new` expression into anything; and I think we need this placement `new` because of the `reinterpret_cast` happening in `dtorArrayTy()`. But it is a bit strange to see the placement `new` hanging off on its own like this. Might be worth some comments explaining. 
> 
> CC @hubert.reinterpretcast @rsmith in case my assessment is incorrect.
I thought using placement new would just call the normal constructors anyway?

BTW, does using a `shared_ptr` here even make sense? Since this is allocated in the `Block`, we need to call the constructor and destructors manually anyway.


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

https://reviews.llvm.org/D154581