This is an archive of the discontinued LLVM Phabricator instance.

IR: Clarify ownership of ConstantDataSequentials, NFC
ClosedPublic

Authored by dexonsmith on Oct 23 2020, 4:45 PM.

Details

Summary

Change ConstantDataSequential::Next to a
unique_ptr<ConstantDataSequential> and update CDSConstants to a
StringMap<unique_ptr<ConstantDataSequential>>, making the ownership
more obvious.

Diff Detail

Event Timeline

dexonsmith created this revision.Oct 23 2020, 4:45 PM
Herald added a project: Restricted Project. · View Herald Transcript
dexonsmith requested review of this revision.Oct 23 2020, 4:45 PM

Is this the right patch/description?

Is this the right patch/description?

Right description / wrong patch :/. Fixed!

aprantl accepted this revision.Oct 26 2020, 9:37 AM

This looks like a good simplification.

This revision is now accepted and ready to land.Oct 26 2020, 9:37 AM
dblaikie added inline comments.Oct 26 2020, 2:57 PM
llvm/lib/IR/Constants.cpp
2787–2788

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.

2795

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

This revision was automatically updated to reflect the committed changes.
dexonsmith added a comment.EditedOct 26 2020, 3:49 PM

Sorry @dblaikie, just saw now (after I pushed) that you commented, I'll read now and decided whether to revert or do a follow-up.

dexonsmith added inline comments.Oct 26 2020, 3:54 PM
llvm/lib/IR/Constants.cpp
2787–2788

Yes, that's a nice cleanup. Thanks for working that through. I'll fix it up in a follow-up.

2795

Good point, I'll update to this.

Thanks for the review.

llvm/lib/IR/Constants.cpp
2787–2788

I cleaned up this (and the other loop) in https://reviews.llvm.org/D90198.

2795

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?

dblaikie added inline comments.Oct 26 2020, 6:06 PM
llvm/lib/IR/Constants.cpp
2787–2788

Great!

2795

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)

dexonsmith added inline comments.Oct 26 2020, 6:20 PM
llvm/lib/IR/Constants.cpp
2795

Right then; I left behind a comment in 52821f6a71a568f966427b627839d40641653757 to clarify for readers.