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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/IR/Constants.cpp:2787-2788
+  std::unique_ptr<ConstantDataSequential> *Entry = &Slot.second;
+  for (ConstantDataSequential *Node = Entry->get(); Node;
+       Entry = &Node->Next, Node = Entry->get())
     if (Node->getType() == Ty)
----------------
dexonsmith wrote:
> dexonsmith wrote:
> > dblaikie wrote:
> > > This becomes a bit fussy to read, what about:
> > > ```
> > > std::unique_ptr<ConstantDataSequential> *Entry = &Slot.second;
> > > for (; *Entry; Entry = &Node->Next)
> > >   if ((*Entry)->getType() == Ty)
> > >     return Entry->get();
> > > ```
> > > 
> > > Does that do the same thing? Perhaps I've missed something/broken it in some way.
> > Yes, that's a nice cleanup. Thanks for working that through. I'll fix it up in a follow-up.
> I cleaned up this (and the other loop) in https://reviews.llvm.org/D90198.
Great!


================
Comment at: llvm/lib/IR/Constants.cpp:2795
+  if (isa<ArrayType>(Ty)) {
+    Entry->reset(new ConstantDataArray(Ty, Slot.first().data()));
+    return Entry->get();
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90083