Proposal: Allow absl's memory.h and port base::WrapUnique to it

[Peter Kasting]

Someone noted the other day that both absl and base have WrapUnique, with the former being contained in absl/memory/memory.h.  I propose we remove the ambiguity by removing the latter in favor of the former.

Allowing all of memory.h will not have much other effect since most of the rest are backports of various STL things we have, e.g. make_unique, or deal with std::weak_ptr and shared_ptr.  Maybe there's something useful in there I missed.

If we allow this, I would make an LSC proposal to bulk-replace one with the other there are currently about 2300 instances of "base::WrapUnique", which seems surprisingly large) and fix the inevitable torrent of ADL and IWYU errors that result.

PK

[Joe Mason]

On Thu, Aug 26, 2021 at 2:07 PM Peter Kasting <pkas...@chromium.org> wrote:

Someone noted the other day that both absl and base have WrapUnique, with the former being contained in absl/memory/memory.h.  I propose we remove the ambiguity by removing the latter in favor of the former.

As mentioned on slack, there are a couple of uses of absl::WrapUnique that have already crept into the codebase. The files they're in only include absl/types/optional.h, which is allowed, so I assume that indirectly includes absl/memory/memory.h, thus defeating the presubmit check for only allowlisted absl headers.

Allowing all of memory.h will not have much other effect since most of the rest are backports of various STL things we have, e.g. make_unique, or deal with std::weak_ptr and shared_ptr.  Maybe there's something useful in there I missed.

Just the RawPtr template, which looks harmless, so +1 to this.

If we allow this, I would make an LSC proposal to bulk-replace one with the other there are currently about 2300 instances of "base::WrapUnique", which seems surprisingly large) and fix the inevitable torrent of ADL and IWYU errors that result.

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDNQPYhstFiDEZS9gcFospnQqskwmNQvf3a6ADiErUx3A%40mail.gmail.com.

[Daniel Cheng]

Hmm. I think it will be a little bit confusing to have two RawPtrs in Chrome. One is the RawPtr template type for MiraclePtr and another will be the RawPtr function from absl.

What do we gain from making this migration? I think there was a strong argument for absl::optional due to the unfortunate conversions required. I don't see quite as much benefit from this proposed change though.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95S1AYfwsRujh2UTSAOG60__3cqXCaWMcSi3ZyvaGc_bzA%40mail.gmail.com.

[Peter Kasting]

On Thu, Aug 26, 2021 at 12:20 PM Daniel Cheng <dch...@chromium.org> wrote:

Hmm. I think it will be a little bit confusing to have two RawPtrs in Chrome. One is the RawPtr template type for MiraclePtr and another will be the RawPtr function from absl.

What do we gain from making this migration? I think there was a strong argument for absl::optional due to the unfortunate conversions required. I don't see quite as much benefit from this proposed change though.

I can't speak to the RawPtr template type for MiraclePtr, as I'm not being kept in the loop on all that :).

The gain here is "not having two valid ways of doing WrapUnique".  It's a small gain; I believed the cost was small as well.

PK

[Łukasz Anforowicz]

On Mon, Aug 30, 2021 at 1:38 PM Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Aug 26, 2021 at 12:20 PM Daniel Cheng <dch...@chromium.org> wrote:

Hmm. I think it will be a little bit confusing to have two RawPtrs in Chrome. One is the RawPtr template type for MiraclePtr and another will be the RawPtr function from absl.

What do we gain from making this migration? I think there was a strong argument for absl::optional due to the unfortunate conversions required. I don't see quite as much benefit from this proposed change though.

I can't speak to the RawPtr template type for MiraclePtr, as I'm not being kept in the loop on all that :).

FWIW, the MiraclePtr's raw_ptr<T> class template is spelled slightly differently from the absl's RawPtr function template, so maybe the naming conflict is not that bad?

The gain here is "not having two valid ways of doing WrapUnique".  It's a small gain; I believed the cost was small as well.

PK

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFB5Y-o0ed4iYMpA1PtS4cFrfosePzFjG5ob8f9aOevrAA%40mail.gmail.com.

[Peter Kasting]

While doing my perf I found this thread, which didn't reach a conclusion.  It sounds like Daniel was slightly concerned and there wasn't much other feedback.  I still feel like this is a small win.  Now that we're further with raw_ptr<>, is the state of the world any clearer?  Can people weigh in?

PK

[dan...@chromium.org]

On Mon, Feb 21, 2022 at 10:26 PM Peter Kasting <pkas...@chromium.org> wrote:

While doing my perf I found this thread, which didn't reach a conclusion.  It sounds like Daniel was slightly concerned and there wasn't much other feedback.  I still feel like this is a small win.  Now that we're further with raw_ptr<>, is the state of the world any clearer?  Can people weigh in?

raw_ptr is landed and experimenting, it will most likely be a vocabulary type going forward in Chrome.

Can we have RawPtr split out of memory.h in absl?

 

PK

On Thu, Aug 26, 2021 at 11:07 AM Peter Kasting <pkas...@chromium.org> wrote:

Someone noted the other day that both absl and base have WrapUnique, with the former being contained in absl/memory/memory.h.  I propose we remove the ambiguity by removing the latter in favor of the former.

Allowing all of memory.h will not have much other effect since most of the rest are backports of various STL things we have, e.g. make_unique, or deal with std::weak_ptr and shared_ptr.  Maybe there's something useful in there I missed.

If we allow this, I would make an LSC proposal to bulk-replace one with the other there are currently about 2300 instances of "base::WrapUnique", which seems surprisingly large) and fix the inevitable torrent of ADL and IWYU errors that result.

PK

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBPa%3DDJoHtiqfYqaFHqQYR9NjCM0gL0qUEgKiD6JA6sOw%40mail.gmail.com.

[Nico Weber]

I agree with Daniel's stance fwiw. Seems like it adds some issues (rawptr confusion), doesn't solve a big problem, and causes a bunch of busiwork.

(Also, moving to make_unique or making code use unique_ptr is most of the time what you want to do instead of WrapUnique, right?)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQuxdZHkfx78wnxufkwEbfUVZH6RXfXWSXE%3DQGtU4-BFg%40mail.gmail.com.