aaron.ballman added inline comments.

================
Comment at: clang/lib/AST/Interp/Descriptor.cpp:42
                         const Descriptor *D) {
+  new (Ptr) InitMapPtr(std::nullopt);
+
----------------
tbaeder wrote:
> 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.
> I thought using placement new would just call the normal constructors anyway?

It should, so I think all the lifetime issues are accounted for, but this area 
of C++ is poorly understood at the best of times.

> 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.

I think it makes sense as we're intentionally sharing the pointer, aren't we? 
So we'll call the ctors/dtors manually in this case, but other objects holding 
the shared pointer don't have to worry about the pointer being yanked out from 
under them.


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

https://reviews.llvm.org/D154581

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to