Best practices when storing a `raw_ref` inside a class

[Jeroen Dhollander]

Hey all,

I'm trying to find the best way to write the constructor for a class that stores a `raw_ref` internally.

Naively I would take a reference in the constructor and pass it to my raw_ref, like this:

```

class Widget { 

  explicit Widget(Item& item) : item_(item) {}

 raw_ref<Item> item_;

};

```

(which is for example done here in chromium).

However, this runs against the Google best practices as stated in http://go/totw/labs/reference-members and http://go/totw/116, which state that you should use a pointer if there are lifetime concerns (which there are as we store the reference).

They actually advice to pass a pointer and do a null check before storing:

```

class Widget { 

  explicit Widget(Item* item) : item_(*ABSL_DIE_IF_NULL(item)) {}

 raw_ref<Item> item_;

};

```

But we don't have `ABSL_DIE_IF_NULL` in our code, so we can't do this.

Which leads to code like this:

```

class Widget { 

  // let's hope we don't get a nullptr as that will crash and burn

  explicit Widget(Item* item) : item_(*item) {}

 raw_ref<Item> item_;

};

```

but I do not like this 'code and pray' approach.

(example in chromium).

Of course we could also add a DCHECK in the body of the constructor, but that fails if the reference is used before that (for example by passing it to the constructor of another member variable).

So what's the best practice here? I can think of a few solutions:

   1) Introduce ABSL_DIE_IF_NULL or something equivalent in Chromium.

   2) Do a null check inside the raw_ref constructor.

   3) Ignore the ToTW and pass references to our constructor.

   4) Pass pointers to our constructors and just pray they are not null.

Personally I prefer options #1 or #2 (and I'd be willing to help make these changes).

With regards,

Jeroen

[Daniel Cheng]

The recommendation is to avoid *const* reference params when there might be lifetime issues. The reason is const reference params can bind to temporaries: https://google.github.io/styleguide/cppguide.html#:~:text=Avoid%20defining%20functions%20that%20require%20a%20const%20reference%20parameter%20to%20outlive%20the%20call%2C%20because%20const%20reference%20parameters%20bind%20to%20temporaries.

There is no such concern for *mtuable* lvalue references, which cannot bind to temporaries.

So it's fine to pass by mutable reference here.

ABSL_DIE_IF_NULL() looks like a useful addition though; however, it is defined in absl/log... though it doesn't look like it will cause us to run into any weird log macro conflicts today, we should probably have a brief discussion on allowing that specific functionality (or cloning it into //base if we feel using any part of absl logging is too risky).

Daniel

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/3022d702-5586-4620-94e0-7ec2113f4d7fn%40chromium.org.

[Jeroen Dhollander]

Thanks for the response, I'll pass in mutable references then (and point any reviewer that has concerns to this thread :)).

Where would be the best place to have a discussion on ABSL_DIE_IF_NULL? I've definitely had code before where I could use that macro so I'm definitely willing to drive introducing this.

Jeroen

[Bartek Nowierski]

I don't wanna derail the discussion too much, but go/totw/labs/reference-members#if-we-do-use-reference-members-how-should-we-pass-them-to-constructors doesn't like non-const references either, saying "While it's true that a non-const lvalue reference will not bind to a temporary, there is still value in adding a speed bump (in the form of pass-by-pointer) to make callers more aware when there are lifetime concerns. [...]"

ABSL_DIE_IF_NULL does sound like the best way to go.

I think this discussion is orthogonal to weather the field is stored as raw_ref<T> or T& inside the class.

[Roland Bock]

So if the guidance is to pass a pointer and we want to pass non-null pointers, and store it in a raw_ref, then we could just as well pass a `raw_ref` (smart pointer for pointer that cannot be null + plus magic).

Granted, that moves the responsibility of construction to the caller, but the caller might be in a better position to determine the validity of the reference.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/72ff36cb-c2f9-46d9-941b-b2ab1f3f4f38n%40chromium.org.

--

--

Roland Bock

Software Engineering Manager

rb...@google.com

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

The above terms reflect a potential business arrangement, are provided solely as a basis for further discussion, and are not intended to be and do not constitute a legally binding obligation. No legally binding obligations will be created, implied, or inferred until an agreement in final form is executed in writing by all parties involved.

[Peter Boström]

I'd prefer we passed the raw_ref<T> in wherever we want it to be non-null, let that spread up the call hierarchy as far as you can to communicate such by type.

Also raw_ref<T>::from_ptr(T* ptr) is landing in crrev.com/c/3949665 (which CHECKs ptr) so I'd prefer using that over constructing a raw_ptr(*ABSL_DIE_IF_NULL(ptr)). Maybe we can consider using similar constructs elsewhere.

Peter

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHq1K7Rf-Ty-M7CgG8xs06qqtTk5PcezUZzArEViJ1u3uk%2BGcg%40mail.gmail.com.

[Peter Boström]

With that in this case I would expect to have explicit Widget(raw_ref<Item> item) being called as Widget(raw_ref<Item>::from_ptr(item)); or if possible move raw_ref<Item>::from_ptr(item) further up the call hierarchy (can item be stored as a raw_ref earlier?).

[Daniel Cheng]

The current guidance is to use C++ native pointers for function params/locals: https://chromium.googlesource.com/chromium/src/+/HEAD/base/memory/raw_ptr.md#pointers-in-locations-other-than-fields

That is because raw_ptr does some extra work under the hood for construction/destruction. It seems like the same usage guidance should probably apply to both?

Daniel

[Peter Boström]

Since communicating non-nullability in the type is nice, would that mean preferring T& up the call stack and assigning that as a *ABSL_DIE_IF_NULL(ptr) high up the stack, use pointers with annotation or just keep ptrs without annotation here?

[dan...@chromium.org]

On Thu, Oct 13, 2022 at 3:10 PM Peter Boström <pb...@chromium.org> wrote:

Since communicating non-nullability in the type is nice, would that mean preferring T& up the call stack and assigning that as a *ABSL_DIE_IF_NULL(ptr) high up the stack, use pointers with annotation or just keep ptrs without annotation here?

I'd kinda like a CHECK_DEREF(x) that returns `*x` instead of the absl version which returns `x`..

But yeah the current guidance is to avoid raw_ptr (and raw_ref) in arguments, so T& would be better representative.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAGFX3sEsNhL_Yy1yL7ba%3D-cxnKMCnpScxJQawsihZdNjyofuhg%40mail.gmail.com.

[Jeroen Dhollander]

I like the idea of `raw_ref::from_ptr` but in reality I feel it looks overly verbose and requires redundantly repeating the reference type when trying to store the raw ref inside a member variable:

```

     // so verbose

      : hover_card_controller_(raw_ref<TabHoverCardController>::from_ptr(hover_card_controller))

     // compared with the current unsafe code
      : hover_card_controller_(*hover_card_controller)

     // or the safe CHECK_DEREF

      : hover_card_controller_(CHECK_DEREF(hover_card_controller))

      // I tried making `from_ptr` templated, but it does not compile:

      : hover_card_controller_(raw_ref::from_ptr(hover_card_controller))

      // however a standalone templated function does compile:

      : hover_card_controller_(raw_ref_from_ptr(hover_card_controller))

     // (alternative names would be `to_raw_ref` or `make_raw_ref`)

```

[Peter Boström]

I put up base::raw_ref_from_ptr() as crrev.com/c/3969541, I'd like more opinions on whether it should be landed. I think it makes sense to use a from_ptr() when assigning directly to a raw_ref, and CHECK_DEREF(ptr) would make more sense when storing it in an intermediate T&.

[Jeroen Dhollander]

I like this idea, thanks for doing this. I will take a stab at introducing CHECK_DEREF during the bugbash next week.

[Roland Bock]

On Fri, Oct 21, 2022 at 9:12 AM Jeroen Dhollander <jero...@google.com> wrote:

I like this idea, thanks for doing this. I will take a stab at introducing CHECK_DEREF during the bugbash next week.

On Fri, Oct 21, 2022 at 12:20 AM Peter Boström <pb...@chromium.org> wrote:

I put up base::raw_ref_from_ptr() as crrev.com/c/3969541, I'd like more opinions on whether it should be landed. I think it makes sense to use a from_ptr() when assigning directly to a raw_ref, and CHECK_DEREF(ptr) would make more sense when storing it in an intermediate T&.

Just for completeness, we could also do something like 

ALWAYS_INLINE explicit raw_ref(T* ptr) noexcept : inner_(ptr) { CHECK(ptr); } 

See crrev.com/c/3971161: I used the same pet example as Peter. The resulting code is even shorter, but of course not as explicit as Peter's suggestion.

[dan...@chromium.org]

On Fri, Oct 21, 2022 at 9:59 AM Roland Bock <rb...@google.com> wrote:

On Fri, Oct 21, 2022 at 9:12 AM Jeroen Dhollander <jero...@google.com> wrote:

I like this idea, thanks for doing this. I will take a stab at introducing CHECK_DEREF during the bugbash next week.

On Fri, Oct 21, 2022 at 12:20 AM Peter Boström <pb...@chromium.org> wrote:

I put up base::raw_ref_from_ptr() as crrev.com/c/3969541, I'd like more opinions on whether it should be landed. I think it makes sense to use a from_ptr() when assigning directly to a raw_ref, and CHECK_DEREF(ptr) would make more sense when storing it in an intermediate T&.

Just for completeness, we could also do something like 

ALWAYS_INLINE explicit raw_ref(T* ptr) noexcept : inner_(ptr) { CHECK(ptr); } 

See crrev.com/c/3971161: I used the same pet example as Peter. The resulting code is even shorter, but of course not as explicit as Peter's suggestion.

The explicitness is a feature IMO.

[Peter Boström]

I'd agree with that, raw_ref(my_foo) reads like my_foo is actually not a ptr. We wouldn't want implicit conversion from T* to T& in general.

[Roland Bock]

Agreed.

[Jeroen Dhollander]

Update on this:

   * CHECK_DEREF landed in https://crrev.com/c/3985828, meaning you can now use this to safely dereference a pointer that's known to be non-null. See the header for more documentation 
   * There are no plans to introduce `base::raw_ref_from_ptr()` or any alternative like this. See discussion here.