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

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

dexonsmith added a comment.

Thanks for the review.



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


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90083