[PATCH] D51525: [ADT] Add a new class OwnedOrBorrowed<T>

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

FWIW, thanks for the fantastic explanation Zach.

I'm still interested in Dave's take on this, but here is mine.

I don't think we should add this general of a construct. While I understand your use case, I *do* think it represents a really awkward design that would be best avoided. That doesn't mean you can reasonably avoid it, but it makes me very hesitant to build very general purpose abstractions that are likely to encourage the same pattern in other bits of code.

I also suspect (but I'm not sure) that there is a better approach when handling this directly within your library. My suggestion would be to (wait for it!) add a layer of abstraction. Fundamentally, you have two implementation strategies that want different ownership semantics. That's fine, but exposing a `std::unique_ptr` in the API that sits *above* that implementation leaks an ownership strategy out of the implementation and into the interface. Instead, I think you should build an abstraction that more thoroughly hides the ownership semantics. This may be effectively the same type as you have here, but now rather than merely being "maybe we own it, maybe we don't", we have a more principled distinction: we're abstracting between implementation A and B where the implementations have different ownership strategies. And if you use some handle type, you may be able to have it "always be destroyed" but only trigger the deallocation when necessary based on the implemnetation details so that the user of the interface cannot observe this "maybe owned" semantic -- they see it as *always* having ownership, its just that in one case the ownership is a trivial wrapper because of a long-lived cache.

Not sure if this makes tons of sense, happy to try nand chat about how to more cleanly figure this out if helpful, but I also don't want to slow down or add more work to the design of the underlying debug info library -- it may just not be worth the time/effort in that localized part of the code to build this more complex construct....


https://reviews.llvm.org/D51525