Pointers vs references

[Avi Drissman]

In https://crrev.com/c/2757372 a function is proposed for the content API (a function I'm cool with) but it takes as a parameter a reference.

I asked for it to be a pointer; as far as I know, we only use pointers for passing around heavyweight objects like RenderWidgetHostViews, BrowserContexts, WebContents, etc. I got feedback that as per the style guide, if a parameter can't be null, a const reference should be used.

I'm going to give feedback that consistency with the existing code is important, but the fact that we aren't quite following this style guide rule is taken. I suppose this is related to the existing const question, but what are peoples' thoughts about this?

Avi

[John Abd-El-Malek]

AFAIK this isn't something that we've had an opinion or tried to enforce in the past. So the fewer differences between content api and google style guide the better. I'd view this as the entire codebase was using pointers and now it's recommended to make new methods take references when possible, and content/public is just a subset of the codebase.

--
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/CACWgwAZn%3DhBZqzb1qqgYcjSk8znGPM-NMAX-7H9yuL%3D_j9dWMg%40mail.gmail.com.

[Avi Drissman]

Hmm. To be clear,

  virtual void CopyBackgroundColorIfPresentFrom(
      const RenderWidgetHostView& other) = 0;

looks good? I'll go along; maybe I'm just old-fashioned with my desire for pointers.

Avi

[Kentaro Hara]

LGTM.

I don't think we need to proactively rewrite the existing code to take references (the benefit is small) but it'd be reasonable to adopt a recommended rule in new code.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CACWgwAZ%3D1b6c1CmYrA5COpDd%2BxO46LLOt1DDR0v7b71qjefO1g%40mail.gmail.com.

--

Kentaro Hara, Tokyo

[Avi Drissman]

Note though:

Suppose that we allow const references to content objects. Now we have a problem. The style guide rule of "you should use const references for non-null objects" requires objects to correctly annotate themselves with const, which is assumed by the style guide but is explicitly disallowed by the content API ruleset.

I don't see how we can have it both ways. I personally think that the removal of the "const" rule from the content API ruleset would be a good thing, and that would allow for the use of const references here, but that's definitely a discussion to be had.

(It happens to work in this particular CL, since only member variables need to be accessed, but it wouldn't work in the general case.)

Avi

[Matt Falkenhagen]

Wouldn't const references and const pointers have the same issue about constness in content?

Where we would have used a non-const pointer for a param in the old days, I think the style guide now says to use a mutable reference if it's non-null.

2021年3月16日(火) 10:17 'Avi Drissman' via content-owners <content...@chromium.org>:

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