Style Guide: Use references for all non-null pointer arguments

[Matt Falkenhagen]

The Blink C++ Style Guide has this rule: "Pointer arguments that can never be null should be passed as a reference, even if this results in a mutable reference argument."

This diverges from Chromium and Google style, which prohibits mutable reference arguments. The last time this came up was this thread, which was several years ago before Chromium and Blink style starting converging. The styles have been converging since then, and there is ongoing work moving code from //content/renderer to //third_party/blink so I'd like to revisit this.

As written, the exception sounds mandatory (you must use a reference). I propose either making the exception optional (permit using a reference) or removing the exception (you must use a pointer).

What do people think?

[Kentaro Hara]

+1 to removing the gap between the two coding style guides. We're going in that direction :)

(Also feel free to file this kind of bug against Hotlist=CodeRefactoring.)

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAJ_xCimGpfAyn3%2BRdQ1B_0FecCYL48_aaCBM-PQijP2he6Oy3w%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

[Hayato Ito]

+1 for "you must use a reference for non-null pointers" in Blink. That's actually we are going in Blink, I think.

If there is a strong reason, "permit using a pointer for non-null pointer" can be acceptable, however, that should be the last resort and should be temporary workaround, and we need a reason for that.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CABg10jwwwGwquGT5-kog46R0NCVwgKa2_oM0uUwHSrgqLCm8Vw%40mail.gmail.com.

--

Hayato

[Yuta Kitamura]

I'm personally reluctant to remove non-const references. Historically, mutable references were introduced to wipe out unnecessary null checks from the codebase -- there were so many pointer arguments whose non-nullness was ambiguous and people just added null checks just "for sure", which added up and caused people's frustration (figuring out whether a specific pointer can be null was super stressful work).

I'm opposed to reversing the gear, because: (1) mixing two conventions would make things worse, and (2) the "phantom null check" problem is still a real issue for us, and it's more desirable for us to ensure the non-nullness by syntax, rather than other unreliable methods like comments or runtime checks.

[I would rather think mutable reference arguments should be allowed in Google C++ style for these reasons. They are part of standard C++, after all.]

On Wed, Feb 13, 2019 at 5:01 PM Matt Falkenhagen <fal...@chromium.org> wrote:

--

[Peter Kasting]

The reason why the Google style guide does not allow non-const ref arguments is that it often makes calling code significantly less clear, since at the callsite these look like pass-by-value.  In personal experience I find this a noticeable readability problem.

I'm sympathetic to not wanting bogus null checks.  I think it's important to distinguish that problem from the related one of trying to prevent people from passing null where they actually shouldn't.  The main advantage that ref arguments have is in preventing this latter problem.  If we entirely ignored this problem, and just looked at "don't add bogus null checks", you don't really need reference args for that, as a simple policy to not add null checks unless you can demonstrate and document the specific case that needs them handles it, and DCHECKs make function preconditions clear.  (I enforce that in all my reviews and see others do the same.)

As to whether "passing null where you shouldn't" is actually a problem worth solving... it does happen, especially when trying to create tests and figure out what mock objects to instantiate and the like.  It doesn't seem like it's all that common to me.  I rank the readability of caller code for ref-taking functions as a much bigger issue.

I'm not on the Blink subteam, but from my Chromium style and ex-Google readability perspective, I think that "const refs for non-mutated args, pointers for mutated args, ban null checks except where the author explains why they're necessary, add DCHECKs to document a function's preconditions" is probably the best mixture overall.

PK

[Chris Harrelson]

I think we should keep Blink code internally consistent, so am not in favor of an "optional" version in the style guide. And unless it's causing specific problems, I propose we leave the style guide as-is and retain this difference between Blink and non-Blink.

Chris

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAAHOzFDo1y5zJQumv6U5SzSV1HypX57YqRa9wMVbJN8J7-P8rQ%40mail.gmail.com.

[Ken Rockot]

I strongly agree with what Peter says here. I think it's important to acknowledge that mutable refs are not the only way to address redundant null checks, and the style guide's policy has good rationale behind it.

PK

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAAHOzFDo1y5zJQumv6U5SzSV1HypX57YqRa9wMVbJN8J7-P8rQ%40mail.gmail.com.

[Dave Tapuska]

I believe some teams have been adding references that is contrary to the style guide, I came across https://chromium.googlesource.com/chromium/src/+/5c568461a49ea21a26597782deec31f3d8fa414a and crbug.com/874385 which promotes this.

dave.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CA%2BapAgG_HLOkDby3gwapBEF1Nv4UxKbyW-c2LdXY3KXWvfSyqA%40mail.gmail.com.

[Peter Kasting]

On Wed, Feb 13, 2019 at 9:57 AM Dave Tapuska <dtap...@chromium.org> wrote:

I believe some teams have been adding references that is contrary to the style guide, I came across https://chromium.googlesource.com/chromium/src/+/5c568461a49ea21a26597782deec31f3d8fa414a and crbug.com/874385 which promotes this.

Since those are in Blink code, those are in keeping with the current Blink style guide.

PK 

[Victor Costan]

On Wed, Feb 13, 2019 at 2:34 AM Yuta Kitamura <yu...@chromium.org> wrote:

I'm personally reluctant to remove non-const references. Historically, mutable references were introduced to wipe out unnecessary null checks from the codebase -- there were so many pointer arguments whose non-nullness was ambiguous and people just added null checks just "for sure", which added up and caused people's frustration (figuring out whether a specific pointer can be null was super stressful work).

I'm opposed to reversing the gear, because: (1) mixing two conventions would make things worse, and (2) the "phantom null check" problem is still a real issue for us, and it's more desirable for us to ensure the non-nullness by syntax, rather than other unreliable methods like comments or runtime checks.

[I would rather think mutable reference arguments should be allowed in Google C++ style for these reasons. They are part of standard C++, after all.]

I would not know how to make a strong argument for changing the Google C++ style to match Blink. There were discussions around this, and the current situation prevailed.

If we can't change Google's C++ style guide, we should align to it. Having to switch between two style guides when writing/reviewing code adds a mental toll, leaving less energy for other tasks. I claim this is a valid concern for every team that has to work on features that span Blink and the browser.

IMHO we should introduce separate types for non-nullable pointers, which would keep us honest with DCHECKs. As far as I know, it's possible to "deref" a null pointer into a null refrence. That being said, I care more about removing style divergence than I care about the specific method we pick to cope with the Google C++ style.

Thank you,

    Victor

[Jeremy Roman]

FWIW, there was an effort a while ago to rewrite *from* pointers *to* references, though I weakly opposed it on roughly these grounds. I do like style consistency, but I'm afraid that we'd just be undoing that work and then in a couple years we'd churn back. :/

nit: Dereferencing a null pointer executes undefined behavior. While a compiler *may* (and usually does) do what you describe, I believe it can also choose to assume that the pointer "must be" non-null at the point of dereference and use that fact to make other optimizations, like optimizing out later code paths that are guarded by null checks, or earlier code paths that would result in the pointer being null at the time you dereference it (since they must be unreachable for the program to not have undefined behavior).

 

Thank you,

    Victor

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAP_mGKqpQrAArudkKvmUNZmFQhR8NRW7vMSiLr1QM%2BKmM4VhJQ%40mail.gmail.com.

[Victor Costan]

On Wed, Feb 13, 2019 at 3:16 PM Jeremy Roman <jbr...@chromium.org> wrote:

FWIW, there was an effort a while ago to rewrite *from* pointers *to* references, though I weakly opposed it on roughly these grounds. I do like style consistency, but I'm afraid that we'd just be undoing that work and then in a couple years we'd churn back. :/

I hope we wouldn't literally revert the changes from references to pointers. I'd like to capture the knowledge we've gained by introducing non-nullable pointer types, and using them to replace mutable references.

While I think it makes sense to weigh benefits (like style consistency) against costs (like churn), I don't think churn should be a major blocker. If we start avoiding churn, we lose the ability to undo decisions that don't play out well. This would significantly limit how much we'd be willing to experiment with new thins, and we ossify into obsolence.

    Victor

[Daniel Cheng]

Somewhat tangential, but let's say we hypothetically decide not to use mutable reference arguments. There's a long-term plan to change a number of getters from this:

class LocalFrame {

 public:

  LocalFrameClient* Client();

  Page* GetPage();

  LocalFrameView* View();

  Document* GetDocument();

};

to:

class LocalFrame {

 public:

  // Callers need to ensure that LocalFrame is attached, either via

  // checking |IsAttached()| explicitly or via some other guaranteed

  // invariant, before using these getters.

  LocalFrameClient& Client();

  Page& GetPage();

  LocalFrameView& View();

  Document& GetDocument();

};

The idea is that code would need to be explicit about checking that the frame isn't detached before accessing Client() / GetPage() / View() / GetDocument(). However, if we started passing arguments by pointer, that means something like this:

  ... = HTMLElement::Create(*GetDocument());

will change to:

  ... = HTMLElement::Create(&GetDocument());

I think the latter looks a bit strange. I'm not sure if this is a good enough argument for keeping the status quo or not though.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAP_mGKrnVSzd8nxh45x6%2Bh3OoFkMWScEeW44hyKaSL%2B8yn1EOA%40mail.gmail.com.

[Peter Kasting]

On Wed, Feb 13, 2019 at 5:21 PM Daniel Cheng <dch...@chromium.org> wrote:

Somewhat tangential, but let's say we hypothetically decide not to use mutable reference arguments. There's a long-term plan to change a number of getters from this:

class LocalFrame {

 public:

  LocalFrameClient* Client();

  Page* GetPage();

  LocalFrameView* View();

  Document* GetDocument();

};

to:

class LocalFrame {

 public:

  // Callers need to ensure that LocalFrame is attached, either via

  // checking |IsAttached()| explicitly or via some other guaranteed

  // invariant, before using these getters.

  LocalFrameClient& Client();

  Page& GetPage();

  LocalFrameView& View();

  Document& GetDocument();

};

While mutable references are allowed as return types, calling code is sometimes clearer if the return types are pointers.

I don't think you need to change the return type to require that callers check that the frame isn't detached (and these functions could DCHECK the latter to document it as a precondition).  So that desire seems a bit orthogonal to the signature change.

That would make your sample caller code:

  ... = HTMLElement::Create(GetDocument());

PK

[Fergal Daly]

On Wed, 13 Feb 2019 at 19:34, Yuta Kitamura <yu...@chromium.org> wrote:

I'm personally reluctant to remove non-const references. Historically, mutable references were introduced to wipe out unnecessary null checks from the codebase -- there were so many pointer arguments whose non-nullness was ambiguous and people just added null checks just "for sure", which added up and caused people's frustration (figuring out whether a specific pointer can be null was super stressful work).

I'm opposed to reversing the gear, because: (1) mixing two conventions would make things worse, and (2) the "phantom null check" problem is still a real issue for us, and it's more desirable for us to ensure the non-nullness by syntax, rather than other unreliable methods like comments or runtime checks.

It's great documentation and I really like that (vs not having it in google3), it definitely makes errors less likely but it's only documentation, it doesn't actually provide real protection. The following code compiles just fine

void bar(String& s) {

  LOG(INFO) << s;

}

void foo(String* s) {

  bar(*s);

}

and segfaults on foo(nullptr) and I have seen real crashes involving null references.

There are other ways to get the same level of documentation but that also give some real protection e.g. NotNull<T> as discussed in the previous thread. These are somewhat verbose but if we want to adopt something, as a project, we could abbreviate any of them, e.g. N<T>. To be clear, I have no opinion on the desirable way to achieve this, I just wanted to point out that it seems achievable and the verbosity argument can be countered somewhat.

Each style guide gives a distinct benefit (documenting mutability vs documenting nullability) but they conflict on how they deliver that benefit by overloading the same feature. Both benefits are worth having, that's why the argument keeps coming up.

Personally, I'd rather have both benefits with some extra verbosity than pick one or the other,

F

 

[I would rather think mutable reference arguments should be allowed in Google C++ style for these reasons. They are part of standard C++, after all.]

On Wed, Feb 13, 2019 at 5:01 PM Matt Falkenhagen <fal...@chromium.org> wrote:

The Blink C++ Style Guide has this rule: "Pointer arguments that can never be null should be passed as a reference, even if this results in a mutable reference argument."

This diverges from Chromium and Google style, which prohibits mutable reference arguments. The last time this came up was this thread, which was several years ago before Chromium and Blink style starting converging. The styles have been converging since then, and there is ongoing work moving code from //content/renderer to //third_party/blink so I'd like to revisit this.

As written, the exception sounds mandatory (you must use a reference). I propose either making the exception optional (permit using a reference) or removing the exception (you must use a pointer).

What do people think?

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAJ_xCimGpfAyn3%2BRdQ1B_0FecCYL48_aaCBM-PQijP2he6Oy3w%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAFJcur9UgReGPBzj%2BC_x%3DaOrPrzejRZN412gKPB2m%3DqPUrd7Jg%40mail.gmail.com.

[Peter Kasting]

On Wed, Feb 13, 2019 at 5:33 PM 'Fergal Daly' via blink-dev <blin...@chromium.org> wrote:

There are other ways to get the same level of documentation but that also give some real protection e.g. NotNull<T> as discussed in the previous thread.

(Note that the Guidelines Support Library provides gsl::not_null<T> for exactly this purpose.)

PK 

[Yoichi Osato]

+1 to add some non_null syntax and avoid raw deref operator "*".

2019年2月14日(木) 10:41 Peter Kasting <pkas...@chromium.org>:

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAAHOzFABb%3DK-Df8HJHrDRLk__Ah6_O1y2X6aN625MCwWYhNWiA%40mail.gmail.com.

[Yuta Kitamura]

On Thu, Feb 14, 2019 at 10:33 AM Fergal Daly <fer...@google.com> wrote:

On Wed, 13 Feb 2019 at 19:34, Yuta Kitamura <yu...@chromium.org> wrote:

I'm personally reluctant to remove non-const references. Historically, mutable references were introduced to wipe out unnecessary null checks from the codebase -- there were so many pointer arguments whose non-nullness was ambiguous and people just added null checks just "for sure", which added up and caused people's frustration (figuring out whether a specific pointer can be null was super stressful work).

I'm opposed to reversing the gear, because: (1) mixing two conventions would make things worse, and (2) the "phantom null check" problem is still a real issue for us, and it's more desirable for us to ensure the non-nullness by syntax, rather than other unreliable methods like comments or runtime checks.

It's great documentation and I really like that (vs not having it in google3), it definitely makes errors less likely but it's only documentation, it doesn't actually provide real protection. The following code compiles just fine

void bar(String& s) {

  LOG(INFO) << s;

}

void foo(String* s) {

  bar(*s);

}

and segfaults on foo(nullptr) and I have seen real crashes involving null references.

The most important effect of mutable reference in function arguments is that it moves the responsibility to do null checks from the called function to the caller. A mutable reference argument lets the user of the function pause and think, "hey, is this pointer really non-null?", before putting an asterisk before it. And this effect not only happens in each function-caller pair, but also in the whole codebase; the non-nullness assertion propagates until it reaches a concrete value or a pointer that's properly null checked. I think this gave the great code health benefit from my experience when WebKit moved to mutable refs. We had a lot of bogus null checks, but also had a lot of nullptr crashes coming from the incorrect assumption that a pointer is non-null. Use of references made us more aware of the (non-)nullness of pointers.

 

There are other ways to get the same level of documentation but that also give some real protection e.g. NotNull<T> as discussed in the previous thread. These are somewhat verbose but if we want to adopt something, as a project, we could abbreviate any of them, e.g. N<T>. To be clear, I have no opinion on the desirable way to achieve this, I just wanted to point out that it seems achievable and the verbosity argument can be countered somewhat.

Each style guide gives a distinct benefit (documenting mutability vs documenting nullability) but they conflict on how they deliver that benefit by overloading the same feature. Both benefits are worth having, that's why the argument keeps coming up.

As I said earlier, the mutable ref argument does something more than documentation; it makes the caller put an asterisk before a pointer, which is a sufficient warning sign to the author and reviewers. I think its code health benefit is very important.

Personally, I'd rather have both benefits with some extra verbosity than pick one or the other,

I agree, but unfortunately it's hard to emulate a mutable reference due to the language constraints around references. I also feel that adding NonNull<T> is like recreating a poor man's reference and it doesn't sound very productive.

In the first place, using a pointer as a mutating argument is a kind of an abuse of the language feature -- references are the C++ way of doing that (unlike C), and the pointers have a critical defect that gives you an unnecessary nullptr state. The badness of null as a language feature is discussed in many places as many of you know.

Perhaps we could add a soft rule that we must always use a simple Mutable() wrapper like the following when we pass mutating arguments? It doesn't prevent accidental ref passing, though... (is this something a Clang plugin can check?)

template <typename T>
T& Mutable(T& t) { return t; }

// Usage:
void f(std::string& s);

std::string s;
f(Mutable(s));

 

[Fergal Daly]

On Thu, 14 Feb 2019 at 14:59, Yuta Kitamura <yu...@chromium.org> wrote:

On Thu, Feb 14, 2019 at 10:33 AM Fergal Daly <fer...@google.com> wrote:

On Wed, 13 Feb 2019 at 19:34, Yuta Kitamura <yu...@chromium.org> wrote:

I'm personally reluctant to remove non-const references. Historically, mutable references were introduced to wipe out unnecessary null checks from the codebase -- there were so many pointer arguments whose non-nullness was ambiguous and people just added null checks just "for sure", which added up and caused people's frustration (figuring out whether a specific pointer can be null was super stressful work).

I'm opposed to reversing the gear, because: (1) mixing two conventions would make things worse, and (2) the "phantom null check" problem is still a real issue for us, and it's more desirable for us to ensure the non-nullness by syntax, rather than other unreliable methods like comments or runtime checks.

It's great documentation and I really like that (vs not having it in google3), it definitely makes errors less likely but it's only documentation, it doesn't actually provide real protection. The following code compiles just fine

void bar(String& s) {

  LOG(INFO) << s;

}

void foo(String* s) {

  bar(*s);

}

and segfaults on foo(nullptr) and I have seen real crashes involving null references.

The most important effect of mutable reference in function arguments is that it moves the responsibility to do null checks from the called function to the caller. A mutable reference argument lets the user of the function pause and think, "hey, is this pointer really non-null?", before putting an asterisk before it. And this effect not only happens in each function-caller pair, but also in the whole codebase; the non-nullness assertion propagates until it reaches a concrete value or a pointer that's properly null checked. I think this gave the great code health benefit from my experience when WebKit moved to mutable refs. We had a lot of bogus null checks, but also had a lot of nullptr crashes coming from the incorrect assumption that a pointer is non-null. Use of references made us more aware of the (non-)nullness of pointers.

 

There are other ways to get the same level of documentation but that also give some real protection e.g. NotNull<T> as discussed in the previous thread. These are somewhat verbose but if we want to adopt something, as a project, we could abbreviate any of them, e.g. N<T>. To be clear, I have no opinion on the desirable way to achieve this, I just wanted to point out that it seems achievable and the verbosity argument can be countered somewhat.

Each style guide gives a distinct benefit (documenting mutability vs documenting nullability) but they conflict on how they deliver that benefit by overloading the same feature. Both benefits are worth having, that's why the argument keeps coming up.

As I said earlier, the mutable ref argument does something more than documentation; it makes the caller put an asterisk before a pointer, which is a sufficient warning sign to the author and reviewers. I think its code health benefit is very important.

Yeha, I really like that we have it and fair enough, it's more than documentation but it's less than real safety.

 

Personally, I'd rather have both benefits with some extra verbosity than pick one or the other,

I agree, but unfortunately it's hard to emulate a mutable reference due to the language constraints around references. I also feel that adding NonNull<T> is like recreating a poor man's reference and it doesn't sound very productive.

In the first place, using a pointer as a mutating argument is a kind of an abuse of the language feature -- references are the C++ way of doing that (unlike C), and the pointers have a critical defect that gives you an unnecessary nullptr state. The badness of null as a language feature is discussed in many places as many of you know.

Perhaps we could add a soft rule that we must always use a simple Mutable() wrapper like the following when we pass mutating arguments? It doesn't prevent accidental ref passing, though... (is this something a Clang plugin can check?)

template <typename T>
T& Mutable(T& t) { return t; }

// Usage:
void f(std::string& s);

std::string s;
f(Mutable(s));

I found an old thread debating this in internal google discussion and searching some more, I found a 2017 thread (Google internal) that was pretty negative about gsl:not_null (and in general the use of types to declare and/or enforce constrains or invariants) and was actually in favour of references for non-null to the extent that it might be time to change the style guide (no idea how realistic that is).

I also found a proposal for a "mut" attribute (again internal only, sorry) that would make it clear in the method signature and at the call-site that mutation may happen (and enforced by tools) although that proposal seems old and unloved.

I have started a thread over in Google c-users to try find out what the current state and plans are for any of this in Google internal C++,

F

 

 

[Adam Rice]

As someone who works on both sides of the Blink divide, I strongly object to this and other gratuitous deviations from Chromium style. Chromium is far larger than just Blink, and the rest of the project has managed to follow Google style without catastrophe. This territorial "not invented here" attitude is an impediment to the velocity of Chromium as a whole.

Yes, it would be nice if we had a safe way to annotate non-null pointers, but mutable references are not it. If anything, they make promises that the compiler doesn't actually keep.

Blink should be aiming to converge on Chromium style, not deviate further.  If we can't reach agreement to remove the mutable references, we should at least permit Chromium style, in order to allow convergence to happen naturally.

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.

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

[Matt Falkenhagen]

I agree with Adam’s points.

My specific problems with the current Blink style guide are: 1) I find the mutable refs confusing for the reasons explained in the Google style guide; 2) there’s cognitive load to remember this exception when regularly working on code across Blink and Chromium; 3) I find it counter-productive to re-style //content code to match this rule when moving it to //third_party/blink particularly as it reduces readability and consistency with the rest of the project.

I’ve seen two code reviews recently where this rule was referenced, which suggests others have trouble “code-switching” when working across the project. In both instances, the people the rule was quoted to were Blink core OWNERS.

In response to the concern about keeping Blink internally consistent, it seems to me Blink is already internally inconsistent. There was an effort started around 2013 to move from pointers to references, and it’s still ongoing as seen in the bug Dave linked to. The “BAD” example in the style guide for FrameLoader is still coded like that today, and a cursory look through Blink turns up other examples. Even if Blink were internally consistent, it still a direct consumer of code in directories like //base, //services, //media, //cc, and its own //third_party/blink/common which use Chromium style.

I think the concern about churn also doesn't need to be blocking. Either way there will be churn. If the exception remains mandatory, the existing pointer code in Blink needs to change, and all the code in //content/renderer needs to change when moved into Blink. Onion Soup has been ongoing for several years, and there are still about 200,000 lines of code in //content/renderer. I don’t know if the long-term plan is still to move all of //content to Blink but that would be even larger churn.

So I still think we should stop requiring divergence from Chromium style here, if not prohibit it.

[Ian Kilpatrick]

At risk of piling on, I'll add my 2cents.

Inside of the LayoutNG codebase (blink/core/layout/ng) we've been following the chromium/google-style of not having any mutable references.

This has (in my opinion) been a very positive thing. There are a few reasons for this:

 

1) It is very typical for layout code to pass things by reference, and by value. For example the LayoutUnit type is typically passed by value. In the existing layout codebase it was impossible what a function did based on just looking at its callsite, e.g. here.

2) It helps minimize side-effect code. Due to the number of concepts and objects layout has to deal with the previous system struggled to keep functions "pure" and many have un-intended side effects. Not having mutable references means that it is very obvious when a function is going to have a side-effect on an object, and if it is actually necessary. I'm positive I've created technical debt by not picking up on calls to these types of functions in code-review.

3) It helps minimize side-effect/technical debt code part 2. Lots of places inside the existing layout system accessed a mutable reference from a class, and performed some side effect on it, which is bad code-structure, and creates technical debt. It is very difficult for a reviewer to pick up on this side effect, when everything just looks like a const reference, and the function names look innocent.

The layout codebase is a has very high code complexity (layout is hard!). It carries a very high cognitive overhead when developing for it. While there is a little bit of overhead to DCHECK a mutable pointer and the start of a function, this is saved by being able to reason about the code sanely.

Ian  

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

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

[Yuta Kitamura]

On Thu, Feb 14, 2019 at 8:36 PM Adam Rice <ri...@chromium.org> wrote:

As someone who works on both sides of the Blink divide, I strongly object to this and other gratuitous deviations from Chromium style. Chromium is far larger than just Blink, and the rest of the project has managed to follow Google style without catastrophe. This territorial "not invented here" attitude is an impediment to the velocity of Chromium as a whole.

Yes, it would be nice if we had a safe way to annotate non-null pointers, but mutable references are not it. If anything, they make promises that the compiler doesn't actually keep.

To reiterate my points: references are almost the ONLY sane way to annotate non-null pointers. They won't prevent nullptr crashes 100%, but they enforce correctness in the syntax and make incorrect dereference much more easily discoverable. I would say its code health benefit was very clear as someone who observed the migration in WebKit.

On the other hand, using a pointer does not seem like the only way to annotate mutability (in my opinion it does more harm than good). I hope the community to seek the possibility to make the Google style better in a constructive manner, rather than shutting out an idea with "rules are rules"-like attitude.

[Hayato Ito]

On the hat of people who prefer "Use references for all non-null pointer arguments" in Blink,

I'd suggest that Blink C++ style guide should explicitly mention about the usage of const references.

I mean:

1. Use const references, |const Foo&|, for all non-null pointer arguments if there is no side-effect on the object

2. or use references, |Foo&|, for all non-null pointer arguments if there is a side-effect on the object

3. or use pointers to const, |const Foo*|, for nullable pointer arguments if there is no side-effect on the object

4. or use pointers, |Foo*|, for nullable pointer arguments if there is a side-effect on the object

Then, both side-effect-ness and nullable-ness would be clear.

I still think it is okay to permit "Use a pointer for non-null pointer" as a temporary workaround.

I can guess easily that it would be super painful to address all style guide violations just in one CL when we convert code from Blink to Chromium, or vice-versa.

That can be addressed in follow-up CLs, I think.

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.

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

--

Hayato

[Ken Rockot]

On Thu, Feb 14, 2019 at 10:38 PM Yuta Kitamura <yu...@chromium.org> wrote:

On Thu, Feb 14, 2019 at 8:36 PM Adam Rice <ri...@chromium.org> wrote:

As someone who works on both sides of the Blink divide, I strongly object to this and other gratuitous deviations from Chromium style. Chromium is far larger than just Blink, and the rest of the project has managed to follow Google style without catastrophe. This territorial "not invented here" attitude is an impediment to the velocity of Chromium as a whole.

Yes, it would be nice if we had a safe way to annotate non-null pointers, but mutable references are not it. If anything, they make promises that the compiler doesn't actually keep.

To reiterate my points: references are almost the ONLY sane way to annotate non-null pointers. They won't prevent nullptr crashes 100%, but they enforce correctness in the syntax and make incorrect dereference much more easily discoverable. I would say its code health benefit was very clear as someone who observed the migration in WebKit.

On the other hand, using a pointer does not seem like the only way to annotate mutability (in my opinion it does more harm than good). I hope the community to seek the possibility to make the Google style better in a constructive manner, rather than shutting out an idea with "rules are rules"-like attitude.

I haven't seen anyone arguing "rules are rules" though. Many would probably agree that divergence from the style guide can be justified when we have compelling arguments. In this case however it seems that many feel the guidance is correct as-is, for the concrete reason that hidden mutability is a noticeable detriment to readability. It's not just some hypothetical problem invented by the style guide maintainers.

What I keep coming back to is this: we're able to maintain suitable code quality and readability (including a general lack of redundant null checks) without mutable refs anywhere else in our very large tree, so what is unique about //third_party/blink that makes this more challenging there?

Follow-up thought: if we're really set on passing mutable refs, could we have a clang plugin enforce that all such values are wrapped in something like a MUT(x) macro we define? Like DoTheThing(x, y, MUT(z))? Not sure it's a feasible or good idea, just... an idea.

Blink should be aiming to converge on Chromium style, not deviate further.  If we can't reach agreement to remove the mutable references, we should at least permit Chromium style, in order to allow convergence to happen naturally.

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.

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

[Yuta Kitamura]

On Fri, Feb 15, 2019 at 4:54 PM Ken Rockot <roc...@chromium.org> wrote:

On Thu, Feb 14, 2019 at 10:38 PM Yuta Kitamura <yu...@chromium.org> wrote:

On Thu, Feb 14, 2019 at 8:36 PM Adam Rice <ri...@chromium.org> wrote:

As someone who works on both sides of the Blink divide, I strongly object to this and other gratuitous deviations from Chromium style. Chromium is far larger than just Blink, and the rest of the project has managed to follow Google style without catastrophe. This territorial "not invented here" attitude is an impediment to the velocity of Chromium as a whole.

Yes, it would be nice if we had a safe way to annotate non-null pointers, but mutable references are not it. If anything, they make promises that the compiler doesn't actually keep.

To reiterate my points: references are almost the ONLY sane way to annotate non-null pointers. They won't prevent nullptr crashes 100%, but they enforce correctness in the syntax and make incorrect dereference much more easily discoverable. I would say its code health benefit was very clear as someone who observed the migration in WebKit.

On the other hand, using a pointer does not seem like the only way to annotate mutability (in my opinion it does more harm than good). I hope the community to seek the possibility to make the Google style better in a constructive manner, rather than shutting out an idea with "rules are rules"-like attitude.

I haven't seen anyone arguing "rules are rules" though. Many would probably agree that divergence from the style guide can be justified when we have compelling arguments. In this case however it seems that many feel the guidance is correct as-is, for the concrete reason that hidden mutability is a noticeable detriment to readability. It's not just some hypothetical problem invented by the style guide maintainers.

Actually I'm not saying mutability annotation is not important, but rather it may be achieved differently without sacrificing null-check-ability.

I apologize if my word sounded too offensive. I generally wanted people to not refuse the mutable ref rules blindly without assessing its technical merits. Someone could have compared the technical merits of references and the importance of the coding style alignment and said the latter would be more important. However, I have not seen anyone who oppose to mutable refs assessing the technical merits of allowing references in detail. WebKit did that for a reason -- I hope people to understand that background and make a fair judgement based on all the discussions from both sides.

I agree that the coding style alignment is important, but I think we shouldn't enforce it as is if it sacrifices the important code health benefit.

 

What I keep coming back to is this: we're able to maintain suitable code quality and readability (including a general lack of redundant null checks) without mutable refs anywhere else in our very large tree, so what is unique about //third_party/blink that makes this more challenging there?

As far as I see it comes from Blink's stronger reliance to heap (pointer-based) objects, including refcounted and garbage-collected (NB. GC didn't exist when WebKit migration happened) ones. We used pointers a lot and were fairly loose on how to use them.

 

Follow-up thought: if we're really set on passing mutable refs, could we have a clang plugin enforce that all such values are wrapped in something like a MUT(x) macro we define? Like DoTheThing(x, y, MUT(z))? Not sure it's a feasible or good idea, just... an idea.

That's essentially same as what I referred as Mutable() before. In theory a Clang plugin should be able to check this, but practically I'm not sure.

 

[Peter Kasting]

On Fri, Feb 15, 2019 at 12:53 AM Yuta Kitamura <yu...@chromium.org> wrote:

I generally wanted people to not refuse the mutable ref rules blindly without assessing its technical merits. Someone could have compared the technical merits of references and the importance of the coding style alignment and said the latter would be more important. However, I have not seen anyone who oppose to mutable refs assessing the technical merits of allowing references in detail.

Well, I tried.  It wasn't tremendously detailed, but it seemed like it was at least as detailed as other comments here.  And I didn't feel like I was the only one.  It seemed like the majority of the comments in this thread, on both sides, addressed the merits of the various practices, beyond simply "align with style guide X".  Maybe I just skimmed the posts that weren't like that, though.

Do you think this summary is fair?:

Using non-const ref arguments is clearer than any alternative (arguably, including gsl::not_null<> and the like) about documenting at the declaration that a function's argument must not be null.  It is the only choice that really adds enforcement of this condition (DCHECK is not meant to serve as runtime enforcement so much as documentation, and even runtime enforcement pales compared to compile-time).  If you believe that ensuring pointers are non-null is the paramount concern, ref arguments clearly achieve it best.

The cost of this is that ref arguments are indistinguishable from value arguments at a callsite, adding potential readability and correctness hazards to callers.  Mitigating this requires cumbersome custom annotations (just as not_null<> would be cumbersome to fix the opposite problem).  They are currently banned by the Google and Chromium style guides, so preserving them also incurs the costs of a fairly impactful style disagreement for anyone working across the boundaries.

Chromium code is nearly consistent about not using non-const ref arguments to functions.  Blink code is less consistent about always using them.  Experienced coders in each group have tended to express a preference for their existing syntax, and at least one person supporting each style has claimed to feel like migrating from one style to the other was a win.

FWIW, I don't see anything in the above as providing definitive support for one side in the eyes of a neutral third party.

What I keep coming back to is this: we're able to maintain suitable code quality and readability (including a general lack of redundant null checks) without mutable refs anywhere else in our very large tree, so what is unique about //third_party/blink that makes this more challenging there?

As far as I see it comes from Blink's stronger reliance to heap (pointer-based) objects, including refcounted and garbage-collected (NB. GC didn't exist when WebKit migration happened) ones. We used pointers a lot and were fairly loose on how to use them.

Chromium seems to use primarily heap allocations too (at least where I work in UI -- everything in Views is pointer-based).  Are you sure Blink is significantly different than Chromium code in this regard?

PK

[dan...@chromium.org]

My impression is that blink has used nullchecks as defensive programming very extensively in the past. It's not uncommon to see something like

if (page && page->document && page->document->frame && page->document->frame->widget)
   page->document->frame->widget->letsgo();

And it's not easy to then untangle which of those are actually possible - sometimes none! The same thing still occurs on member variables, but that's a different topic. It's not that this doesn't happen in "Chromium" code, but it has seemed like much more of a pattern in blink, and trying to undo that by using references seems like it would have been a very effective way since it leverages the compiler in helping track down all callers to a given function.

So if I had to guess what the big difference is, I'd say that Chromium has historically done better overall wrt the law of demeter.

All this said, just yesterday I was reading a method in blink, and a variable was passed to a number of methods, and as innocent as their names sounded, I had to remember to go look at the impl of each method to figure out of the variable was going to get mutated in order to understand what I was reading in the original method. So I myself value the callsites being clear about mutation over the signature being clear about null, while still feeling *very strongly* about defensive branching like null checks and make every one of them be explained usually via comments. (But I am used to google/chromium style I guess too.)

Cheers,

Dana

 

PK

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAAHOzFDuP47mFEi-d5pS37uYQW0OExwetnaGARAE1OmMqZ_%3DTg%40mail.gmail.com.

[Matt Falkenhagen]

On Sat, Feb 16, 2019 at 1:44 AM <dan...@chromium.org> wrote:

On Fri, Feb 15, 2019 at 5:20 AM Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Feb 15, 2019 at 12:53 AM Yuta Kitamura <yu...@chromium.org> wrote:

I generally wanted people to not refuse the mutable ref rules blindly without assessing its technical merits. Someone could have compared the technical merits of references and the importance of the coding style alignment and said the latter would be more important. However, I have not seen anyone who oppose to mutable refs assessing the technical merits of allowing references in detail.

Well, I tried.  It wasn't tremendously detailed, but it seemed like it was at least as detailed as other comments here.  And I didn't feel like I was the only one.  It seemed like the majority of the comments in this thread, on both sides, addressed the merits of the various practices, beyond simply "align with style guide X".  Maybe I just skimmed the posts that weren't like that, though.

Do you think this summary is fair?:

Using non-const ref arguments is clearer than any alternative (arguably, including gsl::not_null<> and the like) about documenting at the declaration that a function's argument must not be null.  It is the only choice that really adds enforcement of this condition (DCHECK is not meant to serve as runtime enforcement so much as documentation, and even runtime enforcement pales compared to compile-time).  If you believe that ensuring pointers are non-null is the paramount concern, ref arguments clearly achieve it best.

The cost of this is that ref arguments are indistinguishable from value arguments at a callsite, adding potential readability and correctness hazards to callers.  Mitigating this requires cumbersome custom annotations (just as not_null<> would be cumbersome to fix the opposite problem).  They are currently banned by the Google and Chromium style guides, so preserving them also incurs the costs of a fairly impactful style disagreement for anyone working across the boundaries.

Chromium code is nearly consistent about not using non-const ref arguments to functions.  Blink code is less consistent about always using them.  Experienced coders in each group have tended to express a preference for their existing syntax, and at least one person supporting each style has claimed to feel like migrating from one style to the other was a win.

FWIW, I don't see anything in the above as providing definitive support for one side in the eyes of a neutral third party.

What I keep coming back to is this: we're able to maintain suitable code quality and readability (including a general lack of redundant null checks) without mutable refs anywhere else in our very large tree, so what is unique about //third_party/blink that makes this more challenging there?

As far as I see it comes from Blink's stronger reliance to heap (pointer-based) objects, including refcounted and garbage-collected (NB. GC didn't exist when WebKit migration happened) ones. We used pointers a lot and were fairly loose on how to use them.

Chromium seems to use primarily heap allocations too (at least where I work in UI -- everything in Views is pointer-based).  Are you sure Blink is significantly different than Chromium code in this regard?

This has been my experience in //content as well (mostly pointer-based).

 

My impression is that blink has used nullchecks as defensive programming very extensively in the past. It's not uncommon to see something like

if (page && page->document && page->document->frame && page->document->frame->widget)
   page->document->frame->widget->letsgo();

And it's not easy to then untangle which of those are actually possible - sometimes none! The same thing still occurs on member variables, but that's a different topic. It's not that this doesn't happen in "Chromium" code, but it has seemed like much more of a pattern in blink, and trying to undo that by using references seems like it would have been a very effective way since it leverages the compiler in helping track down all callers to a given function.

So if I had to guess what the big difference is, I'd say that Chromium has historically done better overall wrt the law of demeter.

All this said, just yesterday I was reading a method in blink, and a variable was passed to a number of methods, and as innocent as their names sounded, I had to remember to go look at the impl of each method to figure out of the variable was going to get mutated in order to understand what I was reading in the original method. So I myself value the callsites being clear about mutation over the signature being clear about null, while still feeling *very strongly* about defensive branching like null checks and make every one of them be explained usually via comments. (But I am used to google/chromium style I guess too.)

These are good points. I guess another factor is WebKit had a stronger bias toward “code should be self-documenting” so had fewer comments where null checks occurred and less documentation of preconditions.

I’m curious why this discussion hasn’t come up before in the context of Onion Soup. Have people moving code from //content been re-styling the code? I tried looking through the last ~15 CLs with “Onion Soup” in their commit descriptions. Most CLs were style-neutral (const references and nullable pointers, conforming to both style guides). 3 CLs clearly added Chromium-style code (non-nullable pointers). A couple were unclear, and there were instances of Chromium-style code in the existing blink code. I didn’t see any CL that added mutable references. [notes]

I don’t see indications that these are intended to be “temporary workarounds”; the code is still written this way well after the CLs landed. So it seems the rule is already being treated as optional.

So what I’m seeing is:

1. Existing code in //third_party/blink is far from consistent wrt to the style guide.

2. Some people are moving code from //content to //third_party/blink and retaining Chromium-style (Onion Soup).

3. Some people are adding new code to //third_party/blink and made the design decision to use Chromium-style (LayoutNG).

4. Some people are converting Chromium-style code in //third_party/blink to Blink-style (https://crbug.com/874385).

This seems suboptimal.

I get that mutable references have been an effective way to counter the problem of phantom null checks in WebKit-derived code but this justification doesn’t seem to apply as strongly to code imported from Chromium or newly written code. I think aligning to the style of the larger project will be the better choice in the long run as more of the latter is added to //third_party/blink.

[Dmitry Gozman]

On Sun, Feb 17, 2019 at 10:13 PM Matt Falkenhagen <fal...@chromium.org> wrote:

On Sat, Feb 16, 2019 at 1:44 AM <dan...@chromium.org> wrote:

On Fri, Feb 15, 2019 at 5:20 AM Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Feb 15, 2019 at 12:53 AM Yuta Kitamura <yu...@chromium.org> wrote:

I generally wanted people to not refuse the mutable ref rules blindly without assessing its technical merits. Someone could have compared the technical merits of references and the importance of the coding style alignment and said the latter would be more important. However, I have not seen anyone who oppose to mutable refs assessing the technical merits of allowing references in detail.

Well, I tried.  It wasn't tremendously detailed, but it seemed like it was at least as detailed as other comments here.  And I didn't feel like I was the only one.  It seemed like the majority of the comments in this thread, on both sides, addressed the merits of the various practices, beyond simply "align with style guide X".  Maybe I just skimmed the posts that weren't like that, though.

Do you think this summary is fair?:

Using non-const ref arguments is clearer than any alternative (arguably, including gsl::not_null<> and the like) about documenting at the declaration that a function's argument must not be null.  It is the only choice that really adds enforcement of this condition (DCHECK is not meant to serve as runtime enforcement so much as documentation, and even runtime enforcement pales compared to compile-time).  If you believe that ensuring pointers are non-null is the paramount concern, ref arguments clearly achieve it best.

The cost of this is that ref arguments are indistinguishable from value arguments at a callsite, adding potential readability and correctness hazards to callers.  Mitigating this requires cumbersome custom annotations (just as not_null<> would be cumbersome to fix the opposite problem).  They are currently banned by the Google and Chromium style guides, so preserving them also incurs the costs of a fairly impactful style disagreement for anyone working across the boundaries.

Chromium code is nearly consistent about not using non-const ref arguments to functions.  Blink code is less consistent about always using them.  Experienced coders in each group have tended to express a preference for their existing syntax, and at least one person supporting each style has claimed to feel like migrating from one style to the other was a win.

FWIW, I don't see anything in the above as providing definitive support for one side in the eyes of a neutral third party.

What I keep coming back to is this: we're able to maintain suitable code quality and readability (including a general lack of redundant null checks) without mutable refs anywhere else in our very large tree, so what is unique about //third_party/blink that makes this more challenging there?

As far as I see it comes from Blink's stronger reliance to heap (pointer-based) objects, including refcounted and garbage-collected (NB. GC didn't exist when WebKit migration happened) ones. We used pointers a lot and were fairly loose on how to use them.

Chromium seems to use primarily heap allocations too (at least where I work in UI -- everything in Views is pointer-based).  Are you sure Blink is significantly different than Chromium code in this regard?

This has been my experience in //content as well (mostly pointer-based).

 

My impression is that blink has used nullchecks as defensive programming very extensively in the past. It's not uncommon to see something like

if (page && page->document && page->document->frame && page->document->frame->widget)
   page->document->frame->widget->letsgo();

And it's not easy to then untangle which of those are actually possible - sometimes none! The same thing still occurs on member variables, but that's a different topic. It's not that this doesn't happen in "Chromium" code, but it has seemed like much more of a pattern in blink, and trying to undo that by using references seems like it would have been a very effective way since it leverages the compiler in helping track down all callers to a given function.

So if I had to guess what the big difference is, I'd say that Chromium has historically done better overall wrt the law of demeter.

All this said, just yesterday I was reading a method in blink, and a variable was passed to a number of methods, and as innocent as their names sounded, I had to remember to go look at the impl of each method to figure out of the variable was going to get mutated in order to understand what I was reading in the original method. So I myself value the callsites being clear about mutation over the signature being clear about null, while still feeling *very strongly* about defensive branching like null checks and make every one of them be explained usually via comments. (But I am used to google/chromium style I guess too.)

These are good points. I guess another factor is WebKit had a stronger bias toward “code should be self-documenting” so had fewer comments where null checks occurred and less documentation of preconditions.

I’m curious why this discussion hasn’t come up before in the context of Onion Soup. Have people moving code from //content been re-styling the code? I tried looking through the last ~15 CLs with “Onion Soup” in their commit descriptions. Most CLs were style-neutral (const references and nullable pointers, conforming to both style guides). 3 CLs clearly added Chromium-style code (non-nullable pointers). A couple were unclear, and there were instances of Chromium-style code in the existing blink code. I didn’t see any CL that added mutable references. [notes]

I don’t see indications that these are intended to be “temporary workarounds”; the code is still written this way well after the CLs landed. So it seems the rule is already being treated as optional.

I totally agree with this. Being substantially different from other style guides, this rule is hard to enforce. Reviewing both content and blink, I often let pointers slip through in blink reviews because it's hard to switch between two mindsets, and I expect some other reviewers miss these as well. Unless we have an automatic style check (like a warning/reminder), enforcing this rule seems unpractical, and existing blink codebase shows that by mixing both references and pointers. Perhaps, owners of some specific blink directories might be able to actually force this, but in general it does not really work today.

- Dmitry

 

So what I’m seeing is:

1. Existing code in //third_party/blink is far from consistent wrt to the style guide.

2. Some people are moving code from //content to //third_party/blink and retaining Chromium-style (Onion Soup).

3. Some people are adding new code to //third_party/blink and made the design decision to use Chromium-style (LayoutNG).

4. Some people are converting Chromium-style code in //third_party/blink to Blink-style (https://crbug.com/874385).

This seems suboptimal.

I get that mutable references have been an effective way to counter the problem of phantom null checks in WebKit-derived code but this justification doesn’t seem to apply as strongly to code imported from Chromium or newly written code. I think aligning to the style of the larger project will be the better choice in the long run as more of the latter is added to //third_party/blink.

Cheers,

Dana

 

PK

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAAHOzFDuP47mFEi-d5pS37uYQW0OExwetnaGARAE1OmMqZ_%3DTg%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

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

[Kentaro Hara]

There are too many opinions on this thread and I'm a bit concerned that the discussion is not converging.

I discussed this in a leads meeting of the architecture team. To move the discussion forward, I'd suggest that dcheng@, dgozman@ and jbroman@ compile all opinions on this thread and make a concrete proposal in one doc. Then they get back to this thread. The three persons have totally different opinions on the topic, so I believe they can make a balanced, reasonable proposal :)

Does that sound reasonable?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAJfDkaFtWfg4-rkDgWmRiSJyj_UeP9hZddB-37cVLGqS9N8E7w%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

[Matt Falkenhagen]

Sounds like a plan. Thanks for taking this up.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CABg10jzHfQV84OoisWi%3DoWRULoEtcAa0nMMtD-BFeQZgOkV5Jg%40mail.gmail.com.

[Matt Falkenhagen]

Hi, it's been a month. Is there any update?

In the meantime, haraken@ can I send you a CL as follows to update the style guide to make the rule optional and link to this discussion thread? Then when there is a decision we can explicitly update the rule and remove the link. This would reflect reality.

Before:

"Pointer arguments that can never be null should be passed as a reference, even if this results in a mutable reference argument.

Note: Even though Google style prohibits mutable reference arguments, Blink style explicitly permits their use."

After:

"Mutable reference arguments are permitted in Blink, in contrast to Google style."

Note: This rule is under discussion at <link to this thread>."

[Kentaro Hara]

Jeremy, Dmitry and Daniel are working on it -- do you have any update?

In the meantime, haraken@ can I send you a CL as follows to update the style guide to make the rule optional and link to this discussion thread?

SGTM.

[Emil A Eklund]

Sounds like a good outcome, thanks everyone!

[Daniel Cheng]

Sorry for missing the replies on this thread. One of dgozman, jbroman, and dcheng has consistently been unable to meet in the last few weeks. We've scheduled time tomorrow to discuss this topic.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CABg10jxSNAnawzttKf5A2vpm%3DrL5-kKR3%3D57z0pTqGuH4w7Ozw%40mail.gmail.com.

[Daniel Cheng]

We've made it optional (to reflect the reality that some teams are ignoring this guidance already/it's causing some pain for Blink onion souping), but we're still trying to figure out a good long-term solution. We're still investigating the alternatives and trying to figure out the long-term solution here.

Daniel

On Wed, May 15, 2019 at 8:24 AM Victor Costan <cos...@gmail.com> wrote:

Any developments here?

Thank you,

    Victor

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

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

[Victor Costan]

Any developments here?

Thank you,

    Victor

On Mon, Apr 1, 2019 at 7:31 PM Daniel Cheng <dch...@chromium.org> wrote:

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

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