[PATCH] D90083: IR: Clarify ownership of ConstantDataSequentials, NFC

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: llvm/lib/IR/Constants.cpp:2795
+  if (isa<ArrayType>(Ty)) {
+    Entry->reset(new ConstantDataArray(Ty, Slot.first().data()));
+    return Entry->get();
----------------
dblaikie wrote:
> dexonsmith wrote:
> > dexonsmith wrote:
> > > dblaikie wrote:
> > > > I'd generally favor `*Entry = std::make_unique...` (less cognitive load - there's no suspicious raw `new` I feel compelled to double-check ("Is that reset function on a unique_ptr, or some other thing that might have more nuanced semantics on raw pointers, etc, etc")) but up to you
> > > Good point, I'll update to this.
> > Ah, I remember why I didn't have this:
> > ```
> >  error: 'operator new' is a protected member of 'llvm::ConstantData'
> > ```
> > Thoughts on whether it's worth working through this?
> oh, fair enough - nothing to do then!
> 
> (it's possible to make "private-like" constructors for use with make_unique - you can do this by having a public constructor with an unconstructible parameter (eg:
> ```
> class t1 {
>   private:
>     struct private_thing { };
>   public:
>     t1(private_thing);
>     std::unique_ptr<t1> make_t1() {
>       return std::make_unique<t1>(private_thing());
>     }
> };
> ```
> (making it fully robust may require even more work - given the above, not sure if you can use templates to extract and then instantiate the "private_thing" type - I'd have to test more, and then maybe give it a private ctor and have it befriend t1.
> 
> But all of this isn't usually worth it, imho)
Right then; I left behind a comment in 52821f6a71a568f966427b627839d40641653757 to clarify for readers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90083