Using content::RenderFrameHost& as a parameter type (reference vs pointer)

[Łukasz Anforowicz]

Hello,

I wanted to note that since r853633 (from danakj@ in Feb 2021) the Chromium C++ Style Guide "recommends using a reference instead of a pointer when it cannot be null". This is relatively new guidance and so we have quite a bit of old code that defaults to using pointers as parameter types (partly because until 2020 using non-const references in parameter types was discouraged by the Google C++ Style Guide).  There is probably no need to proactively convert the old code (given that changing parameter types leads to sprawling, cascading changes), but in some of my CLs I plan to tweak the parameter types where it seems beneficial.

A recent example where the new guidance has been helpful to me is https://crrev.com/c/3221594 and avi@ suggested that I communicate this more broadly.  That CL starts to ask callers of BuildMenu / ShowContextMenu / HandleContextMenu to pass a reference - this is helpful because 1) it clearly documents that the argument cannot be null and 2) forces the callers to make an explicit dereference when converting a pointer to a reference (hopefully thoughtfully analyzing whether such pointer can ever be null).  The second point was particularly helpful for https://crrev.com/c/3221594, because it helped us confidently assess that in all the call chains there is always a non-null frame available.  The first point was in theory achievable with pointers (documenting non-null-ness via comments and/or DCHECKs) but references express the intent more explicitly (leaning on the language and compiler support).

(I also note that pointer-vs-reference is related, but mostly orthogonal to the question of whether const-references can be used or not;  for now `const` is rarely supported in //content/public API and therefore non-const pointer and references are typically used)

Thanks,

-Lukasz

[John Abd-El-Malek]

On Wed, Oct 20, 2021 at 9:49 AM Łukasz Anforowicz <luk...@chromium.org> wrote:

Hello,

I wanted to note that since r853633 (from danakj@ in Feb 2021) the Chromium C++ Style Guide "recommends using a reference instead of a pointer when it cannot be null". This is relatively new guidance and so we have quite a bit of old code that defaults to using pointers as parameter types (partly because until 2020 using non-const references in parameter types was discouraged by the Google C++ Style Guide).  There is probably no need to proactively convert the old code (given that changing parameter types leads to sprawling, cascading changes), but in some of my CLs I plan to tweak the parameter types where it seems beneficial.

A recent example where the new guidance has been helpful to me is https://crrev.com/c/3221594 and avi@ suggested that I communicate this more broadly.  That CL starts to ask callers of BuildMenu / ShowContextMenu / HandleContextMenu to pass a reference - this is helpful because 1) it clearly documents that the argument cannot be null and 2) forces the callers to make an explicit dereference when converting a pointer to a reference (hopefully thoughtfully analyzing whether such pointer can ever be null).

Do you think we should have a recommended policy of enforcing this, e.g. CHECKing any time a pointer is converted to a reference?

 

  The second point was particularly helpful for https://crrev.com/c/3221594, because it helped us confidently assess that in all the call chains there is always a non-null frame available.  The first point was in theory achievable with pointers (documenting non-null-ness via comments and/or DCHECKs) but references express the intent more explicitly (leaning on the language and compiler support).

(I also note that pointer-vs-reference is related, but mostly orthogonal to the question of whether const-references can be used or not;  for now `const` is rarely supported in //content/public API and therefore non-const pointer and references are typically used)

Thanks,

-Lukasz

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

[Avi Drissman]

The immediate question for me as a reviewer was whether we want, in general, to use references for content objects that we're currently passing around with pointers.

If there's no objection to that, then I'll approve the CL and we can all keep that in mind as we write new code and migrate old code.

[dan...@chromium.org]

On Wed, Oct 20, 2021 at 1:00 PM Avi Drissman <a...@chromium.org> wrote:

The immediate question for me as a reviewer was whether we want, in general, to use references for content objects that we're currently passing around with pointers.

If there's no objection to that, then I'll approve the CL and we can all keep that in mind as we write new code and migrate old code.

Well, anything else would be a divergence from something the style guide says pretty explicitly. We wouldn't want every other component in the codebase picking their styleguide rules to use or ignore.

I will also note that CHECKing derefs isn't a security benefit since we compile with non-standard flags to allow null references to exist without UB (they will still crash, and a CHECK would as well).

[John Abd-El-Malek]

On Wed, Oct 20, 2021 at 1:08 PM <dan...@chromium.org> wrote:

On Wed, Oct 20, 2021 at 1:00 PM Avi Drissman <a...@chromium.org> wrote:

The immediate question for me as a reviewer was whether we want, in general, to use references for content objects that we're currently passing around with pointers.

If there's no objection to that, then I'll approve the CL and we can all keep that in mind as we write new code and migrate old code.

Well, anything else would be a divergence from something the style guide says pretty explicitly. We wouldn't want every other component in the codebase picking their styleguide rules to use or ignore.

+1

I will also note that CHECKing derefs isn't a security benefit since we compile with non-standard flags to allow null references to exist without UB (they will still crash, and a CHECK would as well).

Yep to explain my previous comment more I was thinking of having earlier runtime crashes when this happens, so that if an error occurs we don't find out later and only in some codepaths (this was the cause of the webview omg).