Thoughts on duplicate const/non-const getters

[Sasha Bermeister]

It sounds like there have been discussions on this in the past, so thought I'd try and get the record straight regarding duplicate getter methods with the same name (one const, one non-const).

Context is the following patch, which removes these duplicate methods from Page:

https://codereview.chromium.org/2876393002/

Is this an artifact we no longer want in Blink? Or a design pattern we want to use more? The reasons against doing it are clear (there are half as many methods, for example) but the reasons for it are not obvious to me.

Thanks,

Sasha

[Kentaro Hara]

On Sun, May 14, 2017 at 11:18 PM, Sasha Bermeister <sas...@chromium.org> wrote:

It sounds like there have been discussions on this in the past, so thought I'd try and get the record straight regarding duplicate getter methods with the same name (one const, one non-const).

Context is the following patch, which removes these duplicate methods from Page:

https://codereview.chromium.org/2876393002/

The CL is changing:

  const VisualViewport& GetVisualViewport() const;

to:

  VisualViewport& GetVisualViewport() const;

The fact that the change doesn't hit a compile error indicates that the change is just fine in terms of const-correctness... doesn't it?

If the change is breaking const-correctness, why doesn't it hit a compile error?

 

Is this an artifact we no longer want in Blink? Or a design pattern we want to use more? The reasons against doing it are clear (there are half as many methods, for example) but the reasons for it are not obvious to me.

Thanks,

Sasha

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CADAgNuo7U-HMZ6s5iiT0AQW%3D6O5jLR%3D%2BeGXJPK%3DVdqruzjuZsQ%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

[Yuki Shiino]

In the CL, at least some of members are

    const Member<T> member_;

and we have the following in platform/heap/Member.h.

    template <typename T>

    class MemberBase {

      T& operator*() const { return *raw_; }

    };

where operator*() is a const member function, but returns a non-const reference.  So, it's a kind of

    T* const raw_;

Is this expected and/or am I understanding correctly?

Probably this is a problem of:

    const T*

    T* const

    const T* const

My humble opinion is that "const thing is quite conceptual, so design things with less surprising" and "as long as less surprising, |mutable| is acceptable, but don't abuse it".

Cheers,

Yuki Shiino

2017-05-15 16:09 GMT+09:00 Kentaro Hara <har...@chromium.org>:

On Sun, May 14, 2017 at 11:18 PM, Sasha Bermeister <sas...@chromium.org> wrote:

It sounds like there have been discussions on this in the past, so thought I'd try and get the record straight regarding duplicate getter methods with the same name (one const, one non-const).

Context is the following patch, which removes these duplicate methods from Page:

https://codereview.chromium.org/2876393002/

The CL is changing:

  const VisualViewport& GetVisualViewport() const;

to:

  VisualViewport& GetVisualViewport() const;

The fact that the change doesn't hit a compile error indicates that the change is just fine in terms of const-correctness... doesn't it?

If the change is breaking const-correctness, why doesn't it hit a compile error?

Is this an artifact we no longer want in Blink? Or a design pattern we want to use more? The reasons against doing it are clear (there are half as many methods, for example) but the reasons for it are not obvious to me.

Thanks,

Sasha

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

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

To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CADAgNuo7U-HMZ6s5iiT0AQW%3D6O5jLR%3D%2BeGXJPK%3DVdqruzjuZsQ%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jz%2Bi42PTU4_PFK2xZ7Fqc4YVMnVU6SZOFFYXXjRQE2mDw%40mail.gmail.com.

[Yuta Kitamura]

On Mon, May 15, 2017 at 4:09 PM, Kentaro Hara <har...@chromium.org> wrote:

On Sun, May 14, 2017 at 11:18 PM, Sasha Bermeister <sas...@chromium.org> wrote:

It sounds like there have been discussions on this in the past, so thought I'd try and get the record straight regarding duplicate getter methods with the same name (one const, one non-const).

Context is the following patch, which removes these duplicate methods from Page:

https://codereview.chromium.org/2876393002/

The CL is changing:

  const VisualViewport& GetVisualViewport() const;

to:

  VisualViewport& GetVisualViewport() const;

The fact that the change doesn't hit a compile error indicates that the change is just fine in terms of const-correctness... doesn't it?

Look at this const:

  VisualViewport& GetVisualViewport() const;

This basically means that, if you have a const Page object, you will be able to get mutable VisualViewport& out of it. Semantically, this is very weird (you can change the internal of the Page with a const Page object).

If you remove the const (== simply removing the const overloads from ToT) and the code still compiles, I'm okay with the change, because that means the const overloads aren't used. Otherwise, I'm against it.

I think we should do either:

(1) Remove all the "const" qualifications attached to Page so that const correctness won't matter; or

(2) Live with the current code, because the two overloaded functions have their own roles and they are not really duplicated.

I guess (1) is a lot of work and it sounds like moving backward. I vote on keeping those overloads.

[Dave Tapuska]

I personally agree with keeping these overloads. I agree that it feels like removing them is a step backwards.

Here is an example I can write today to convert a const Page into a const Page. (because VisualViewport breaks const correctness)

Page* GetNonConstPage(const Page* page)  {

  page->GetVisualViewport.GetPage();

}

Yet perhaps the caller doesn't actually expect Page to change inside the callee but it can.

dave.

--

You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

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

[Nico Weber]

Please consider having general, non-blink-specific C++ discussions on c...@chromium.org instead. (In this case you've cc'd pkasting, who I think has opinions on this, but on cxx@ other non-blink people might chime in too.)

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

To post to this group, send email to platform-architecture-dev@chromium.org.

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

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAHXv1wn5wtSq2Wc%3D4c-_U_Hwxx2J8RpLwx5jqBxtx23Dp-%3D7_A%40mail.gmail.com.

[Peter Kasting]

On Mon, May 15, 2017 at 12:34 AM, Yuta Kitamura <yu...@chromium.org> wrote:

On Mon, May 15, 2017 at 4:09 PM, Kentaro Hara <har...@chromium.org> wrote:

On Sun, May 14, 2017 at 11:18 PM, Sasha Bermeister <sas...@chromium.org> wrote:

It sounds like there have been discussions on this in the past, so thought I'd try and get the record straight regarding duplicate getter methods with the same name (one const, one non-const).

Context is the following patch, which removes these duplicate methods from Page:

https://codereview.chromium.org/2876393002/

The CL is changing:

  const VisualViewport& GetVisualViewport() const;

to:

  VisualViewport& GetVisualViewport() const;

The fact that the change doesn't hit a compile error indicates that the change is just fine in terms of const-correctness... doesn't it?

No.

The compiler can only check physical constness.  What we care about as programmers is logical constness.  So compile errors are in general not a good check on whether you "did const correctly".

In this particular case, the change effectively guarantees the maximum caller flexibility, so it's guaranteed to compile, whether or not it makes sense.  So we have to look deeper.

Look at this const:

  VisualViewport& GetVisualViewport() const;

This basically means that, if you have a const Page object, you will be able to get mutable VisualViewport& out of it. Semantically, this is very weird (you can change the internal of the Page with a const Page object).

Exactly.

For this reason, the rule of thumb is that const methods should not return non-const pointers/references.  Returning such often enables the caller to make state changes that are somehow visible to the const object.   It is not worth trying to think deeply about the cases for which that's not true (and can't be true as the code is modified in the future); hence the rule of thumb.

I think we should do either:

(1) Remove all the "const" qualifications attached to Page so that const correctness won't matter; or

(2) Live with the current code, because the two overloaded functions have their own roles and they are not really duplicated.

Note that because constness and the lack thereof are both viral, (1) means that other people cannot use const Page objects as much, which in turn can prevent those callers from being const, and so forth.  Usually (1) is only the route you'd want to go when you think "const" is not worth its costs and should be globally removed from the codebase.

In short, I wouldn't land this CL as-is.

(P.S. I also agree with Nico that "what should good C++ practice be" is maybe better raised on cxx@ than with random private people, as it would let other smart people chime in, which in my case usually means they have good counterarguments to mine.)

PK

[Daniel Cheng]

Sorry, I was the one who suggested the list of people to bootstrap the discussion. =)

The fact of the matter is that today, Blink has many cross-references between DOM objects and many of these are exposed as const member functions that return non-const pointers/references. If we try to implement full const-correctness, we'll have to double up a lot of simple methods. Do we expect this to prevent bugs?

Also, what's the difference between this and things like std::unique_ptr::get()/WTF::RefPtr::Get(), which are const but return a mutable pointer? I guess one could argue that doing ptr.get()->Next()->Prev()->Mutate() to bypass const is an obvious abuse?

Daniel

 

PK

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

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

[Peter Kasting]

On Mon, May 15, 2017 at 5:18 PM, Daniel Cheng <dch...@chromium.org> wrote:

The fact of the matter is that today, Blink has many cross-references between DOM objects and many of these are exposed as const member functions that return non-const pointers/references. If we try to implement full const-correctness, we'll have to double up a lot of simple methods. Do we expect this to prevent bugs?

Your question boils down to "do we expect const-correctness to prevent bugs".  I would read the underlying question as "is const-correctness worth doing".  Google style in general seems to say yes; see https://google.github.io/styleguide/cppguide.html#Use_of_const .  See also Effective C++, item 3.

The question of whether to retrofit "good practice" into a large existing codebase -- whether the effort pays the opportunity cost, and does not introduce new bugs -- is more complicated.  As someone who's not a core OWNER of Blink, that's not my decision to make.  However, many years ago, when I was even more gung-ho about "fix ALL the things", I did specifically recommend doing it to the core DOM classes in WebKit.  :)

Also, what's the difference between this and things like std::unique_ptr::get()/WTF::RefPtr::Get(), which are const but return a mutable pointer?

I don't know, as I wasn't part of the standardization discussion.  Note, however, that a "const unique_ptr<T>" is akin to a "T* const" and not a "const T*", so if you're thinking of unique_ptr analogously to a pointer -- which is, I think, the intent -- you don't really have a "pointer to const object" when you declare your unique_ptr const.  For that, I believe (without testing) you want "unique_ptr<const T>", which will vend you a const T* from get(), and which will block you from doing the sequence you described, as long as Next()/Prev()/Mutate() are const-correct.

PK

[Sasha Bermeister]

+c...@chromium.org for more input.

--

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

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFAnS6d9MZa5cqJACV3r1h2X-1%3DQg92CHCnWBBDOQ7xD3g%40mail.gmail.com.

[Matt Giuca]

+1 to what Peter has said so far.

> I believe (without testing) you want "unique_ptr<const T>", which will vend you a const T* from get(), and which will block you from doing the sequence you described

I just tested this and it's correct.

You can't always guarantee perfectly the const correctness of things within an interface, but you should strive to have the public interface reflect conceptual const correctness. Consider:

class MyClass {

 public:

  // This is bad because a conceptually-const object is exposing a non-const

  // refernce to its internal field.

  int& x() const { return *x_; }

 private:

  // This is "fine" because only MyClass has access to the value. It's the code

  // inside MyClass's responsibility to *not* modify |*x_| from a const method.

  std::unique_ptr<int> x_;

};

This code compiles, but is not conceptually const-correct.

The ideal thing to do here would be to change |x_| to std::unique_ptr<const int>. Then the compiler checks const-correctness for you (the getter becomes a compile error and you realise that you need to make it return a const int*). However, that means |x_| has to always be const, not just from const methods. (You couldn't make a const and non-const getter.) So it's fine to keep |x_| as a non-const int, as long as you expose a const-correct interface:

  int& x() { return *x_; }

  const int& x() const { return *x_; }

Note: In my opinion, you should also strive to avoid non-const accessors wherever possible (the int& x() version). Even though it's const-correct, it violates encapsulation by exposing a member of MyClass for arbitrary modification by the client. MyClass has no way to control the value of its own variables. This isn't a hard rule; there are cases where this is useful, but you should prefer setters and mutation methods over non-const getters. In the CL in question it's not really feasible to remove these, though (since they have so many existing clients). I would just keep Page.h as-is.

On Tue, 16 May 2017 at 10:57 Sasha Bermeister <sas...@chromium.org> wrote:

+c...@chromium.org for more input.

On Tue, May 16, 2017 at 10:43 AM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, May 15, 2017 at 5:18 PM, Daniel Cheng <dch...@chromium.org> wrote:

The fact of the matter is that today, Blink has many cross-references between DOM objects and many of these are exposed as const member functions that return non-const pointers/references. If we try to implement full const-correctness, we'll have to double up a lot of simple methods. Do we expect this to prevent bugs?

Your question boils down to "do we expect const-correctness to prevent bugs".  I would read the underlying question as "is const-correctness worth doing".  Google style in general seems to say yes; see https://google.github.io/styleguide/cppguide.html#Use_of_const .  See also Effective C++, item 3.

The question of whether to retrofit "good practice" into a large existing codebase -- whether the effort pays the opportunity cost, and does not introduce new bugs -- is more complicated.  As someone who's not a core OWNER of Blink, that's not my decision to make.  However, many years ago, when I was even more gung-ho about "fix ALL the things", I did specifically recommend doing it to the core DOM classes in WebKit.  :)

Also, what's the difference between this and things like std::unique_ptr::get()/WTF::RefPtr::Get(), which are const but return a mutable pointer?

I don't know, as I wasn't part of the standardization discussion.  Note, however, that a "const unique_ptr<T>" is akin to a "T* const" and not a "const T*", so if you're thinking of unique_ptr analogously to a pointer -- which is, I think, the intent -- you don't really have a "pointer to const object" when you declare your unique_ptr const.  For that, I believe (without testing) you want "unique_ptr<const T>", which will vend you a const T* from get(), and which will block you from doing the sequence you described, as long as Next()/Prev()/Mutate() are const-correct.

PK

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

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFAnS6d9MZa5cqJACV3r1h2X-1%3DQg92CHCnWBBDOQ7xD3g%40mail.gmail.com.

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

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

[Sasha Bermeister]

Reading through the responses, there are clearly good arguments for both sides. Writing const-correct code would undoubtedly give us useful compile-time errors that ensure the code is behaving as expected, but in reality much of blink isn't currently written in a const-correct way and it would be an enormous (and likely non-trivial) effort to refactor the rest of the codebase to do this.

So the question becomes... Do we want to aim for a const-correct codebase, adding PRESUBMIT checks and style guide sections to explain the new requirements (e.g. every mutable field needs 2 getters, one const one non-const)? This would also mean we would eventually need a project to backport the rest of the codebase to be const correct. Also, many classes would not be able to be written const-correctly because of the legacy code they interact with, as explained above.

The alternative is to aim for consistency, making getters return const objects where they are not modified and non-const where they are, potentially adding const paths to non-const code (but keeping the code simple and straightforward).

I don't mind either, but leaving Page.h as the confusing outlier feels wrong unless we have a plan forward.

S

+c...@chromium.org for more input.

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFAnS6d9MZa5cqJACV3r1h2X-1%3DQg92CHCnWBBDOQ7xD3g%40mail.gmail.com.

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

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

[Kentaro Hara]

On Mon, May 22, 2017 at 1:03 PM, Sasha Bermeister <sas...@chromium.org> wrote:

Reading through the responses, there are clearly good arguments for both sides. Writing const-correct code would undoubtedly give us useful compile-time errors that ensure the code is behaving as expected, but in reality much of blink isn't currently written in a const-correct way and it would be an enormous (and likely non-trivial) effort to refactor the rest of the codebase to do this.

So the question becomes... Do we want to aim for a const-correct codebase, adding PRESUBMIT checks and style guide sections to explain the new requirements (e.g. every mutable field needs 2 getters, one const one non-const)? This would also mean we would eventually need a project to backport the rest of the codebase to be const correct. Also, many classes would not be able to be written const-correctly because of the legacy code they interact with, as explained above.

The alternative is to aim for consistency, making getters return const objects where they are not modified and non-const where they are, potentially adding const paths to non-const code (but keeping the code simple and straightforward).

I'd prefer this route.

Const-correctness is already seriously broken in Blink and I'm not sure if it's worth an effort to fix them.

 

+c...@chromium.org for more input.

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

To post to this group, send email to platform-architecture-dev@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFAnS6d9MZa5cqJACV3r1h2X-1%3DQg92CHCnWBBDOQ7xD3g%40mail.gmail.com.

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

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

To post to this group, send email to platform-architecture-dev@chromium.org.

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

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

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

To post to this group, send email to platform-architecture-dev@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CADAgNuo6bep%2BhbHUcS2Z8WAt1ko9%3DYnwYUGmL71vKrsJo0aGGw%40mail.gmail.com.

[Matt Giuca]

I think we need to distinguish between three classes of correctness here (I'm not sure what the correct terminology is):

• "Const correct": More or less everything is const if it doesn't have to be modified. Methods that don't modify an object are const. This is "ideal" but it's totally arguable whether the benefits outweigh the cost of refactoring.
• "Const okay" (for lack of a better term): There are places where const could be used, but it isn't. But when const is used, it makes sense. Some methods are not const even though they don't modify the object. Some variables are not const even though they could be. But when a method does call itself "const", it respects that declaration. It doesn't modify its object. It doesn't const-cast, and it doesn't make semantic modifications that technically satisfy the compiler (e.g., a const method modifying an object through an owned non-const pointer).
• "Const broken": Methods declare themselves as "const", but they modify the object anyway, or return objects that can later be used to modify the object.

In the case of getters, you have:

• Non-const getter returns non-const object; const getter returns const object: This is "const correct".
• Non-const getter returns non-const object only: This is "const okay"; you need a non-const ref of an object just to read it, which means clients need to unnecessarily ask for write access. This is bad, but nobody is lying.
• Const getter returns non-const object: This is "const broken". The interface is a lie; the client does not need to ask for write access to an object, but they can modify its state anyway.

It sounds like Blink is currently in a "const okay" state. This particular class is "const correct". If you want to change it to "const okay" for consistency (i.e., remove the const getter), then that is fine, but your CL is proposing to make it "const broken".

Kentaro: When you say "Const-correctness is already seriously broken in Blink" -- do you mean Blink code is commonly "const broken" by my definition above, or merely "const okay"?

The fact that there are lots of clients that access those getters from a const object prevents you from simply removing the const getter. That suggests that at least some parts of Blink are operating in a const correct way. Const correctness is not all-or-nothing. I would suggest a useful goal to aim for is "const okay" everywhere, and "const correct" as much as possible, i.e., aim for it in new code but don't make a big effort to refactor old code around it. If you are writing new code that can't be const correct because of old code, don't bother to make the new code const correct, but avoid being "const broken" because that's confusing for everyone.

+c...@chromium.org for more input.

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFAnS6d9MZa5cqJACV3r1h2X-1%3DQg92CHCnWBBDOQ7xD3g%40mail.gmail.com.

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

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

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

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

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

[Yuta Kitamura]

On Mon, May 22, 2017 at 1:03 PM, Sasha Bermeister <sas...@chromium.org> wrote:

Reading through the responses, there are clearly good arguments for both sides. Writing const-correct code would undoubtedly give us useful compile-time errors that ensure the code is behaving as expected, but in reality much of blink isn't currently written in a const-correct way and it would be an enormous (and likely non-trivial) effort to refactor the rest of the codebase to do this.

So the question becomes... Do we want to aim for a const-correct codebase, adding PRESUBMIT checks and style guide sections to explain the new requirements (e.g. every mutable field needs 2 getters, one const one non-const)? This would also mean we would eventually need a project to backport the rest of the codebase to be const correct. Also, many classes would not be able to be written const-correctly because of the legacy code they interact with, as explained above.

Regarding the statement above, I'm afraid you misunderstood something? The statement is too generalized and actually wrong.

I'm not sure what you exactly meant by "mutable field", but in general, a mutable field will get one getter and one setter. That's the usual situation.

The actual problem occurs when you want your class T to return a pointer or a reference to T's subobject. If const T returns a mutable pointer or reference, that essentially means you can modify the internal state of T, which breaks the mental promise of "const T", and this is undesirable. const T returning a const pointer is OK. Non-const T returning a non-const pointer is also OK. If one of those aren't necessary, providing only either version is also OK.

To summarize:

• If a getter does not return a pointer or a reference, just having a const getter is usually sufficient.
• If T's getter returns a pointer or a reference to a subobject of T:
• Define a non-const getter and a const getter returning a non-const pointer (or reference) and a const pointer, respectively.
• If one of those aren't needed, you can omit it.
• Otherwise, there's no general advice that can be applied universally. You probably need to think about it on a case-by-case basis.

This is a common C++ idiom and not something we'd like to put into the style guide. See also a classic book "Effective C++", look for an item called "Avoid returning "handles" to internal data from const member functions."

I think it's possible to make the codebase gradually const correct, if we allow to use const_cast on the const correctness boundary.

I'm not sure whether your argument "there are some classes that cannot be written const-correctly" is true. Ultimately, it should be possible to use const_cast on the legacy boundary and make the most of our codebase const correct.

 

The alternative is to aim for consistency, making getters return const objects where they are not modified and non-const where they are, potentially adding const paths to non-const code (but keeping the code simple and straightforward).

I don't get what you meant here. Can you elaborate?

 

+c...@chromium.org for more input.

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

To post to this group, send email to platform-architecture-dev@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFAnS6d9MZa5cqJACV3r1h2X-1%3DQg92CHCnWBBDOQ7xD3g%40mail.gmail.com.

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

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

To post to this group, send email to platform-architecture-dev@chromium.org.

[Matt Giuca]

On Mon, 22 May 2017 at 15:56 Yuta Kitamura <yu...@chromium.org> wrote:

On Mon, May 22, 2017 at 1:03 PM, Sasha Bermeister <sas...@chromium.org> wrote:

Reading through the responses, there are clearly good arguments for both sides. Writing const-correct code would undoubtedly give us useful compile-time errors that ensure the code is behaving as expected, but in reality much of blink isn't currently written in a const-correct way and it would be an enormous (and likely non-trivial) effort to refactor the rest of the codebase to do this.

So the question becomes... Do we want to aim for a const-correct codebase, adding PRESUBMIT checks and style guide sections to explain the new requirements (e.g. every mutable field needs 2 getters, one const one non-const)? This would also mean we would eventually need a project to backport the rest of the codebase to be const correct. Also, many classes would not be able to be written const-correctly because of the legacy code they interact with, as explained above.

Regarding the statement above, I'm afraid you misunderstood something? The statement is too generalized and actually wrong.

I'm not sure what you exactly meant by "mutable field", but in general, a mutable field will get one getter and one setter. That's the usual situation.

The actual problem occurs when you want your class T to return a pointer or a reference to T's subobject. If const T returns a mutable pointer or reference, that essentially means you can modify the internal state of T, which breaks the mental promise of "const T", and this is undesirable. const T returning a const pointer is OK. Non-const T returning a non-const pointer is also OK. If one of those aren't necessary, providing only either version is also OK.

To summarize:

• If a getter does not return a pointer or a reference, just having a const getter is usually sufficient.
• If T's getter returns a pointer or a reference to a subobject of T:
• Define a non-const getter and a const getter returning a non-const pointer (or reference) and a const pointer, respectively.
• If one of those aren't needed, you can omit it.
• Otherwise, there's no general advice that can be applied universally. You probably need to think about it on a case-by-case basis.

This is a common C++ idiom and not something we'd like to put into the style guide. See also a classic book "Effective C++", look for an item called "Avoid returning "handles" to internal data from const member functions."

I think it's possible to make the codebase gradually const correct, if we allow to use const_cast on the const correctness boundary.

I agree it's possible to make the codebase gradually const correct (and isn't a big deal if this takes years, or never finishes). I don't think we should be encouraging const_cast at all.

I'm not an expert on this and my understanding is hazy. But what I've picked up is that basically const_cast usually results in undefined behaviour, the very bad kind. When you const_cast, the compiler is able to make assumptions that aren't actually true. If someone can elaborate on this, it would be nice. But the rule of thumb I've learned is that const_cast is pretty much always a mistake.

I'd advocate for a more conservative transition to const correctness, as I described previously: if you're using an object that has non-const methods (even though they have no side effects), then too bad, just take a non-const pointer to it, and so on and so on. But for new classes, make their methods const if applicable, and then new code won't have to ask for non-const access.

+c...@chromium.org for more input.

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFAnS6d9MZa5cqJACV3r1h2X-1%3DQg92CHCnWBBDOQ7xD3g%40mail.gmail.com.

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

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

[Kentaro Hara]

On Mon, May 22, 2017 at 2:53 PM, Matt Giuca <mgi...@chromium.org> wrote:

I think we need to distinguish between three classes of correctness here (I'm not sure what the correct terminology is):

• "Const correct": More or less everything is const if it doesn't have to be modified. Methods that don't modify an object are const. This is "ideal" but it's totally arguable whether the benefits outweigh the cost of refactoring.
• "Const okay" (for lack of a better term): There are places where const could be used, but it isn't. But when const is used, it makes sense. Some methods are not const even though they don't modify the object. Some variables are not const even though they could be. But when a method does call itself "const", it respects that declaration. It doesn't modify its object. It doesn't const-cast, and it doesn't make semantic modifications that technically satisfy the compiler (e.g., a const method modifying an object through an owned non-const pointer).
• "Const broken": Methods declare themselves as "const", but they modify the object anyway, or return objects that can later be used to modify the object.

In the case of getters, you have:

• Non-const getter returns non-const object; const getter returns const object: This is "const correct".
• Non-const getter returns non-const object only: This is "const okay"; you need a non-const ref of an object just to read it, which means clients need to unnecessarily ask for write access. This is bad, but nobody is lying.
• Const getter returns non-const object: This is "const broken". The interface is a lie; the client does not need to ask for write access to an object, but they can modify its state anyway.

It sounds like Blink is currently in a "const okay" state. This particular class is "const correct". If you want to change it to "const okay" for consistency (i.e., remove the const getter), then that is fine, but your CL is proposing to make it "const broken".

Kentaro: When you say "Const-correctness is already seriously broken in Blink" -- do you mean Blink code is commonly "const broken" by my definition above, or merely "const okay"?

The fact that there are lots of clients that access those getters from a const object prevents you from simply removing the const getter. That suggests that at least some parts of Blink are operating in a const correct way. Const correctness is not all-or-nothing. I would suggest a useful goal to aim for is "const okay" everywhere, and "const correct" as much as possible, i.e., aim for it in new code but don't make a big effort to refactor old code around it. If you are writing new code that can't be const correct because of old code, don't bother to make the new code const correct, but avoid being "const broken" because that's confusing for everyone.

The current Blink is in a "const broken" state. For example, there are a lot of const_casts in the code base. Also I guess there would be many cases where const getters are returning non-const refs.

My suggestion is:

- Force the "const okay" rule for new code. "const correctness" is nicer but you don't need to try hard to do that.

- Don't need to eagerly refactor the existing code to achieve the perfect "const okay" state.

(I understand that this suggestion might be a bit controversial because forcing the "const okay" rule won't really help as long as the "const broken" code is left in the code base.)

+c...@chromium.org for more input.

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFAnS6d9MZa5cqJACV3r1h2X-1%3DQg92CHCnWBBDOQ7xD3g%40mail.gmail.com.

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

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

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

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

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CADAgNuo6bep%2BhbHUcS2Z8WAt1ko9%3DYnwYUGmL71vKrsJo0aGGw%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

--

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

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAHqYdcaHFeHtBWm9PzrhSFPhzeEyHrWPywB6W8No%3DzMMWmyuag%40mail.gmail.com.

[Yuta Kitamura]

I agree that we have a lot of broken pieces of code out there. But that doesn't justify making the code "more broken".

[Kentaro Hara]

Yeah, so my suggestion is: let's not break the code more by forcing the "const okay" rule for new code :D

[Yuta Kitamura]

On Mon, May 22, 2017 at 3:04 PM, Matt Giuca <mgi...@chromium.org> wrote:

I agree it's possible to make the codebase gradually const correct (and isn't a big deal if this takes years, or never finishes). I don't think we should be encouraging const_cast at all.

I'm not an expert on this and my understanding is hazy. But what I've picked up is that basically const_cast usually results in undefined behaviour, the very bad kind. When you const_cast, the compiler is able to make assumptions that aren't actually true. If someone can elaborate on this, it would be nice. But the rule of thumb I've learned is that const_cast is pretty much always a mistake.

I know dereferencing a non-const pointer casted from a const one is undefined behavior, but I'm not aware of a compiler doing something unexpected when it sees such code. There are already so many casual const_casts out there even in Chromium, and I doubt compilers can break the implicit semantic contract here. (This is from my limited experience; I will happily get corrected by someone more knowledgable)

I'm not saying const_cast is OK, though. It's bad, and should be avoided if possible, but there are cases where const_cast is useful/inevitable.

[Stuart Langley]

Using const_cast is how Meyer's suggests avoiding duplication when implementing const and non-const overloads (In effective C++) - Generally this should be safe as long as it's not making it possible to modify a value in a RO segment.

const Foo& Bar::GetFoo() const {

 // Potentially lots of logic to get a Foo&.

}

Foo& Bar::GetFoo() {

  return const_cast(static_cast<const Foo&>(*this).GetFoo());

}

It would be nice for greenfield code to be const correct, because I believe it improves maintainability. I'm not sure what haraken@ means by "const okay" vs "const correct" tho.

[Kentaro Hara]

It would be nice for greenfield code to be const correct, because I believe it improves maintainability. I'm not sure what haraken@ means by "const okay" vs "const correct" tho.

I agree that "const correct" is better than "const okay". I was wondering if forcing "const correct" might be a bit too much. I'm not sure if it's worth an effort to add a lot of duplicated code for const & non-const overloads or using the const_cast pattern to avoid the duplication.

By the way:

- What is Chromium doing?

- Is there any mechanical way to force the "const correct" rule?

- Is there any mechanical way to force the "const okay" rule?

[Peter Kasting]

On Mon, May 22, 2017 at 5:01 AM, Kentaro Hara <har...@chromium.org> wrote:

- What is Chromium doing?

Chromium is mostly a mix of "const correct" and "const okay".  I don't believe there's much "const broken" stuff, though I've run into a little bit in the past.

 

- Is there any mechanical way to force the "const correct" rule?

- Is there any mechanical way to force the "const okay" rule?

Maybe with clang style checks.  Not with compiler errors about constness, since even "const okay" is not the same as physical constness.

PK

[Matt Giuca]

Stuart: "I'm not sure what haraken@ means by "const okay" vs "const correct" tho."

I invented and defined those terms previously (for lack of any proper terms that I can find). The C++ FAQ "defines" const correctness but it doesn't really, it just says using const is good. It's not clear to me whether the general usage of this term refers to "using const as much as possible" (what I called "const correct") or merely "not letting const be abused" (what I called "const okay"). I'm happy to have these terms replaced by more commonly accepted terms.

On mechanical checks to force: I think the difference between "const correct" and "const okay" isn't really mechanical, it's a design question. Can you design your APIs to limit write access where it isn't necessary? You can't really mechanically enforce "const correct", because "const okay" doesn't violate any rules.

You can't really even enforce "const okay" because "const broken" is a semantic violation, not a physical violation (that would be caught by the compiler).

I would say we should work towards enforcing "const okay" in code reviews, and clean up any "const broken" code. "Const correct" should be encouraged for new code but doesn't need a big "refactor the world" effort.

[Kentaro Hara]

I would say we should work towards enforcing "const okay" in code reviews, and clean up any "const broken" code. "Const correct" should be encouraged for new code but doesn't need a big "refactor the world" effort.

If there's no objection, shall we go with this guideline?

[Stuart Langley]

Sgtm. Is there already guidance in the style guide or shall we add something?

[Peter Kasting]

On Tue, May 23, 2017 at 5:11 PM, Stuart Langley <slan...@chromium.org> wrote:

Sgtm. Is there already guidance in the style guide or shall we add something?

The style guide already encourages the use of const "wherever it makes sense" ( http://google.github.io/styleguide/cppguide.html#Use_of_const ).  It's not clear to me that we need to write more?

PK

[Stuart Langley]

The style guide does call out "and do not return a non-const pointer or non-const reference to a data member." which is the crux of the original CL - so you're probably right that this is enough.

[Matt Giuca]

Stuart, that quote is a little out of context... it doesn't say not to do that, it says: "Other methods should be const if they ... do not return a non-const pointer or non-const reference to a data member." So you are allowed to have a non-const method that returns a non-const reference (which is what Page.h currently has, in addition to the const version).

It also does say "Accessors should almost always be const.", implying that having a non-const accessor that returns a mutable pointer is bad style. However, it seems to be the norm in Blink code (?). That's a "const correct vs const okay" issue, which I don't think is urgent. I am mostly concerned about getting the codebase from "const broken" to "const okay".

What the style guide doesn't mention anything about is the concept of "logical const", where you have an object that is logically a part of its parent object, but because const is non-transitive, its constness isn't enforced by the compiler. I think it's reasonable for our style guide to say we should treat those members as if they were const, which would permit both the "const& getter() const" and the "& getter()" versions of accessors on such objects, but prohibit the "& getter() const" version that's been proposed in Sasha's CL. Suggested wording (perhaps under the Types heading):

"Members of a const object should be logically considered to be const themselves (even though this isn't enforced by the compiler). Avoid modifying these members or exposing them in a modifiable way (e.g., returning a non-const pointer/reference to a class's unique_ptr member from a const method)."

WDYT?

[Peter Kasting]

On Tue, May 23, 2017 at 6:11 PM, Matt Giuca <mgi...@chromium.org> wrote:

What the style guide doesn't mention anything about is the concept of "logical const", where you have an object that is logically a part of its parent object, but because const is non-transitive, its constness isn't enforced by the compiler. I think it's reasonable for our style guide to say we should treat those members as if they were const, which would permit both the "const& getter() const" and the "& getter()" versions of accessors on such objects, but prohibit the "& getter() const" version that's been proposed in Sasha's CL. Suggested wording (perhaps under the Types heading):

"Members of a const object should be logically considered to be const themselves (even though this isn't enforced by the compiler). Avoid modifying these members or exposing them in a modifiable way (e.g., returning a non-const pointer/reference to a class's unique_ptr member from a const method)."

I find this text more confusing than helpful.

If we wanted to add a Chromium style rule, I'd propose the rule I did before, which is what I ask for in reviews: just don't return any non-const pointer or ref from a const method.  This rule is simple to understand.

Admittedly, this is occasionally more than what logical constness requires.  However, the Google style guide's rule (never return non-const pointers/refs to a data member from a const method) can be less than what's required in cases involving global state, or objects which provide accessors to other objects they don't actually own.  (More detailed argument available on request.)

If we need to explain logical constness, I'd explain it as follows:

"Physical constness guarantees that callers do not modify the bits in an object.  Logical constness guarantees that callers do modify the object or any state affecting it in a detectable way, nor gain access permissions to do so.  The two definitions partly overlap.  The compiler enforces physical constness, whereas we seek to enforce logical constness.  The "mutable" keyword, used correctly, is about compiling code that is logically but not physically const.  The style rule (above) is about avoiding code that is physically but not logically const."

PK

[Peter Kasting]

On Tue, May 23, 2017 at 7:31 PM, Peter Kasting <pkas...@chromium.org> wrote:

"Physical constness guarantees that callers do not modify the bits in an object.  Logical constness guarantees that callers do modify the object or any state affecting it in a detectable way, nor gain access permissions to do so.

As Mike Lawther just noted to me (thanks Mike!), I dropped a "not" on the floor in that second sentence.  Should have been "...that callers do not modify...".

That might make it more sensible :)

PK 

[Matt Giuca]

I agree my paragraph is hard to parse and I'd be happy to over-extend to simply "don't return non-const pointers/references from a const method". I think if a const method returns a non-const pointer to something that isn't conceptually part of itself, well that's not a constness problem but it is confusing and something else is probably wonky. So I'm happy to rule that out.

On the other side, this rule doesn't cover const methods modifying their own logically-const members. But I think that's more of an obvious issue that doesn't need a rule.

The paragraph about physical constness vs logical constness sounds perfect to me, modulo missing nots.

[Ryan Hamilton]

Is this something we're thinking of adding to the style guide, or the c++ dos and dont's?

--
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 post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFARb2xSfKZnpJsBYNJnA19OuMXHpxnexfKKoxEZF_T5cw%40mail.gmail.com.

[Stuart Langley]

Thanks Matt - confirmation bias had me interpreting that sentence as never return non-const pointer and references from a const method :)

I agree with Peter - the end state we want is "just don't return any non-const pointer or ref from a const method.  This rule is simple to understand". Perhaps we just massage the existing sentence to remove the "to a data member" part?

[Matt Giuca]

I agree.

Btw (unrelated to this conclusion but discussed earlier in this thread): I did some digging on the undefinedness of const_cast. TL;DR, it's well-defined to a) cast a non-const pointer/ref to const and back again to non-const, then modify it, and b) cast a const pointer/ref to non-const, but not modify it. It's undefined to cast a thing that was originally const to non-const, then modify it. So Stuart's above suggestion of writing the non-const-to-non-const getter in terms of the const-to-const getter (from Meyers) is sound: "const_cast(static_cast<const Foo&>(*this).GetFoo());".

https://stackoverflow.com/questions/25406818/is-the-following-use-of-const-cast-undefined-behavior

Goes into this exact example and the answer cites Meyer (and explains why it's slightly better to define the non-const-to-non-const getter in terms of the const-to-const getter, not the other way around).

The actual definition in the spec is in §7.1.6.1 (page 158 of the November 2014 working draft, a page number that's easy to memorize if you are a Myst fan, which I am), with a good example in clause 4. The spec distinguishes between const-qualified objects and const-qualified types. An object is const if it was declared const at creation time (as a local or member variable, or #til using the "new const" operator). When you const cast (explicitly or implicitly), the type qualifier changes but the object qualifier does not. The compiler doesn't know statically whether a pointer/reference-to-const actually points to a const object, because it might've been cast. It's well-defined to take a pointer-to-const type that points to a non-const object, cast away const, then modify it. It's undefined to modify a const object.

[Peter Kasting]

On Tue, May 23, 2017 at 8:11 PM, Ryan Hamilton <r...@chromium.org> wrote:

Is this something we're thinking of adding to the style guide, or the c++ dos and dont's?

I'm OK with adding nothing to anything, but if we do add something, I suggest adding it to the Dos and Donts page, not the styleguide.

If we change our styleguide, I think we should only do so for factors that differ from google3 in some way.  This isn't really one.  There are legitimate reasons someone can choose to return a non-const pointer from a const object (though I've never encountered a case in Chromium where we were forced to do this).  So this is basically "rule of thumb that will avoid violating the Google style guide and avoid any other potential problems", which sounds to me like a Dos and Donts-level rule.

PK

[Matt Giuca]

It seems appropriate to add something to the Dos and Don'ts page. I agree this doesn't belong in the style guide (Google or Chromium).

[Anthony Berent]

The trouble with a rule saying that "don't return non-const pointers/references from a const method"  is that it tends to propagate constness much too far, and hence prevent its use. 

For example WebContents::GetBrowserContents is a const member returning a pointer to a non-const object. If it returned a const BrowserContents* then, with the current definition of BrowserContents, the only callable members of the returned object would then be its two const members; GetPath, and IsOffTheRecord. While some of the other members probably could be made const I suspect that when we get a BrowserContents from a WebContents we really do want to modify either the BrowserContents or things referenced by it. With the rule "don't return non-const pointers/references from a const method" the only solution would be to make WebContents::GetBrowserContents non-const.

Logically, where an object owns another object then it makes sense for const getters to return const pointers to that object. If, however, an object only references another object, then this does not generally make sense. Requiring this can force the widespread removal of const qualifiers.

--

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 post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHqYdca0a-VpL%3D6W9ixAP2AUy2kfYu-_JiJxsLdcK1rvecRywg%40mail.gmail.com.

[Peter Kasting]

On Thu, May 25, 2017 at 2:37 AM, Anthony Berent <abe...@chromium.org> wrote:

The trouble with a rule saying that "don't return non-const pointers/references from a const method"  is that it tends to propagate constness much too far, and hence prevent its use. 

For example WebContents::GetBrowserContents is a const member returning a pointer to a non-const object. If it returned a const BrowserContents* then, with the current definition of BrowserContents, the only callable members of the returned object would then be its two const members; GetPath, and IsOffTheRecord. While some of the other members probably could be made const I suspect that when we get a BrowserContents from a WebContents we really do want to modify either the BrowserContents or things referenced by it. With the rule "don't return non-const pointers/references from a const method" the only solution would be to make WebContents::GetBrowserContents non-const.

If callers really do want to modify BrowserContext and things it references, and thus the solution is not "make more of BrowserContext's members const, then yes, making WebContents::GetBrowserContext() non-const is correct, and desirable.  It's not "propagating constness too far", and preventing the use of "const" here is desirable, not a bug.

Also, one could argue that NavigationControllerImpl::GetBrowserContext() should not be marked "const" per the Google style guide's rule, because it's returning a data member (not just some other object it could get at) that's a non-const pointer.  If you disagree with that reading of the Google style guide (e.g. by saying "the pointer is the data member, and you're not returning a pointer or reference to it"), then certainly in many cases (e.g. if NavigationControllerImpl owned the browser context) you'd still get "stuck" with the solution you don't like.  And then there's the question of how NavigationControllerImpl got this non-const pointer to begin with; see more below.

The upshot is, I think even if you try to scale back the proposal here to the Google style guide, you likely will run into the same constraints that you dislike about this proposal.

Logically, where an object owns another object then it makes sense for const getters to return const pointers to that object. If, however, an object only references another object, then this does not generally make sense.

I disagree.  Generally, if an object references another object, then changes on the other object can affect the behavior of the current object (otherwise there would be little reason for one to reference the other).

For example, if we have:

Bar* Foo::bar() const { return GetGlobalBar(); }

int Foo::GetVal() const { return bar()->val(); }

Then it's surprising that you could do:

const Foo* foo = ...;

printf("%d", foo->GetVal());

foo->bar()->SetVal(27);

printf("%d", foo->GetVal());

...and have the two lines print different values even though |foo| is const.

More abstractly, saying "you can return a non-const pointer to an object you don't own" allows the following (this is a little more like the navigation controller case above):

class A, B, C;

class A {

 public:

  A() : b_(base::MakeUnique<B>()), c_(base::MakeUnique<C>(b_.get())) {}

  // These const getters return const pointers, because A owns the objects.

  const B* b() const { return b_.get(); }

  const C* c() const { return c_.get(); }

 private:

  std::unique_ptr<B> b_;

  std::unique_ptr<C> c_;

};

class C {

 public:

  // C takes a non-owning pointer because something in it needs to access B.

  explicit C(B* b) : b_(b) {}

  // This pointer can be non-const because C does not own B.

  B* b() const { return b_; }

 private:

  B* b_;

};

void caller() {

  const A a;

  // Despite having only a const A, which should only allow us const access to B, we can bypass the protection.

  B* non_const_b = a.c()->b();

  ...

}

If C never needs to modify B, then this case could be prevented by having C() take a "const B*".  But that's not always a reasonable design constraint, and even if it were, it's much harder to notice and call out as a reviewer than the "don't return non-const pointers from const methods" rule.

Requiring this can force the widespread removal of const qualifiers.

I would argue that it's better to not have const qualifiers than to have them in cases like what you describe.

PK

[Anthony Berent]

On Thu, 25 May 2017 at 19:43 Peter Kasting <pkas...@chromium.org> wrote:

On Thu, May 25, 2017 at 2:37 AM, Anthony Berent <abe...@chromium.org> wrote:

The trouble with a rule saying that "don't return non-const pointers/references from a const method"  is that it tends to propagate constness much too far, and hence prevent its use. 

For example WebContents::GetBrowserContents is a const member returning a pointer to a non-const object. If it returned a const BrowserContents* then, with the current definition of BrowserContents, the only callable members of the returned object would then be its two const members; GetPath, and IsOffTheRecord. While some of the other members probably could be made const I suspect that when we get a BrowserContents from a WebContents we really do want to modify either the BrowserContents or things referenced by it. With the rule "don't return non-const pointers/references from a const method" the only solution would be to make WebContents::GetBrowserContents non-const.

If callers really do want to modify BrowserContext and things it references, and thus the solution is not "make more of BrowserContext's members const, then yes, making WebContents::GetBrowserContext() non-const is correct, and desirable.  It's not "propagating constness too far", and preventing the use of "const" here is desirable, not a bug.

Also, one could argue that NavigationControllerImpl::GetBrowserContext() should not be marked "const" per the Google style guide's rule, because it's returning a data member (not just some other object it could get at) that's a non-const pointer.  If you disagree with that reading of the Google style guide (e.g. by saying "the pointer is the data member, and you're not returning a pointer or reference to it"), then certainly in many cases (e.g. if NavigationControllerImpl owned the browser context) you'd still get "stuck" with the solution you don't like.  And then there's the question of how NavigationControllerImpl got this non-const pointer to begin with; see more below.

The upshot is, I think even if you try to scale back the proposal here to the Google style guide, you likely will run into the same constraints that you dislike about this proposal.

Logically, where an object owns another object then it makes sense for const getters to return const pointers to that object. If, however, an object only references another object, then this does not generally make sense.

I disagree.  Generally, if an object references another object, then changes on the other object can affect the behavior of the current object (otherwise there would be little reason for one to reference the other).

For example, if we have:

Bar* Foo::bar() const { return GetGlobalBar(); }

int Foo::GetVal() const { return bar()->val(); }

Then it's surprising that you could do:

const Foo* foo = ...;

printf("%d", foo->GetVal());

foo->bar()->SetVal(27);

printf("%d", foo->GetVal());

...and have the two lines print different values even though |foo| is const.

Taking this example and making it more concrete; suppose we have 

void Company::setCeo(Person ceo) { ceo_ = ceo;}

Person* Company::GetCeo() const { return ceo_;}

int Company::GetCeoAge() const { return GetCeo()->getAge();}

const Company company = ...

printf("%d", company->GetCeoAge());

foo->getCeo()->SetAge(27);

printf("%d", company->GetCeoAge());

I don't think it is particularly surprising that the CEO's age can change without it changing the company. The CEO's age is not a property of the company, but a property of the person. In fact making GetCeo non-const seems strange, since asking who a company's CEO is should not change the company.

[Peter Kasting]

On Thu, May 25, 2017 at 1:37 PM, Anthony Berent <abe...@chromium.org> wrote:

Taking this example and making it more concrete; suppose we have 

void Company::setCeo(Person ceo) { ceo_ = ceo;}

Person* Company::GetCeo() const { return ceo_;}

int Company::GetCeoAge() const { return GetCeo()->getAge();}

const Company company = ...

printf("%d", company->GetCeoAge());

foo->getCeo()->SetAge(27);

printf("%d", company->GetCeoAge());

I don't think it is particularly surprising that the CEO's age can change without it changing the company. The CEO's age is not a property of the company, but a property of the person. In fact making GetCeo non-const seems strange, since asking who a company's CEO is should not change the company.

Calling an accessor is not just "asking who a company's CEO is" (that would be more like company->getCeoName() or company->GetCeo()->GetName()), but getting a handle to the CEO to do things with.  Basically, you're saying "I want a relationship with the CEO for some reason".  To which the appropriate question is "to do what?"

One might want to merely interview the CEO, in which case the proper accessor looks like:

const Person* Company::GetCeo();

Now you can GetCeo() and ask questions like "how old are you", and they clearly don't change the state of the world.

But other operations do affect things:

company->GetCeo()->GiveBribe();  // Probably affects |company|'s behavior!

If you want to GiveBribe() to the CEO (apologies for the crass example), it would be appropriate for Company to also have a non-const accessor:

Person* Company::GetCeo();

Parallel const and non-const accessors is a common and appropriate tool for "sometimes people need one kind of relationship, sometimes another".  (And C++ is much more limited than real life in the relationships it can model, so clearly const and non-const are a crude approximation to the shades of relationship things can have.)

BTW, even in your original example, consider a later version of the program that adds new, more realistic event handling for age changes:

Person::SetAge(int age) {

  age_ = age;

  if (age_ > kRetirementAge)

    Retire();  // The implementation of this is going to modify the company by changing its CEO!

}

So suddenly we are modifying the company by doing what we thought was a change scoped to another object.

I freely acknowledge that there really are cases where "don't return non-const pointers from const functions" is overly conservative.  But a surprising number of the cases that _look_ overly conservative are more like the above -- they result in changes to the "const" object's apparent state due to coupling/sharding.

PK

[Matt Giuca]

Yep, I'm convinced by that last example that we shouldn't allow const objects to return non-const refs even to things they don't own (agree with Peter).

> The CEO's age is not a property of the company, but a property of the person.

The problem though is that you were never given a non-const handle to the person. You were only given a const handle to the company, and managed to magically fabricate a non-const pointer to the person. I agree that Peter's rule overreaches a bit in cases that might be "fine", but this example shows that you can actually modify an object that nobody intended for you to modify. I think it's reasonable to say that if your method does something funky with the object's internal pointers and ends up returning a modifiable object, that that method does not claim to be "const".

> In fact making GetCeo non-const seems strange, since asking who a company's CEO is should not change the company.

You seem to be suggesting that non-const is a claim that it changes the object. It isn't... it's a lack of a claim that the object won't be changed. If something is dubious, we should err on the side of not making false guarantees (i.e, not call something const if it might allow changes to the object).

And also, it's a simpler rule rather than having to go into the detail of owned vs non-owned pointers.

Note that this is a proposed recommendation in "Dos and Don'ts" (I believe). We aren't going to initiate a major cleanup that results in widespread addition or removal of const throughout the codebase. There will be things grandfathered in just because it's so hard to change a little bit without infecting more code. But we should have this guideline for new code.

[Daniel Cheng]

As it seems I'm in the minority here, what's the recommended idiom for implementing const / non-const overloads that aren't simply trivial getters? It's not great to copy and paste the method body, but const_cast is kind of ugly too.

Daniel

P.S. I've heard that one trick is to add a const helper that returns a non-const object that the two public versions delegate to... that doesn't seem much better =P

[Ryan Hamilton]

Do you have an example of such a method? I'm struggling to come up with such an use-case but I'm sure it'll be obvious once you point it out...

[Daniel Cheng]

https://chromium-review.googlesource.com/c/523925/2/third_party/WebKit/Source/web/WebViewImpl.cpp is one example.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAJ_4DfTzVOkpYKTXPww4oEE77JnZ3emwLG3k06%3D1NW6wr%2B7GwA%40mail.gmail.com.

[Nicholas Verne]

I agree copying and pasting method bodies is undesirable. We could probably come up with a concise macro to generate the definitions, given a one-liner body.

Nick

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

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

[Peter Kasting]

On Mon, Jun 5, 2017 at 2:33 PM, Daniel Cheng <dch...@chromium.org> wrote:

https://chromium-review.googlesource.com/c/523925/2/third_party/WebKit/Source/web/WebViewImpl.cpp is one example.

I would use the const_cast idiom in all cases like this.  If it became common, I'd define a macro or template for it.

PK

[Nicholas Verne]

Here's an ugly macro that works

#define DEFINE_GETTERS(ReturnType, Method, Expr)\

ReturnType Method() { \

  return (Expr);\

}\

\

const ReturnType Method() const {\

  return (Expr);\

}\

--

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

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFBNdo14kpqVcd8HHRnOAL1iEm%2BBx%2B_emeq9NCsfFgr%2B%2BQ%40mail.gmail.com.

[Peter Kasting]

On Mon, Jun 5, 2017 at 2:57 PM, 'Nicholas Verne' via cxx <c...@chromium.org> wrote:

Here's an ugly macro that works

#define DEFINE_GETTERS(ReturnType, Method, Expr)\

ReturnType Method() { \

  return (Expr);\

}\

\

const ReturnType Method() const {\

  return (Expr);\

}\

This is limited to when fitting the method body in one expression is doable and readable.

Then we have the StackOverflow overengineered solution: https://gist.github.com/BlueSolei/bca26a8590265492e2f2760d3cefcf83

Honestly, I think this kind of thing is rare enough that I'd just use the const_cast idiom directly.  But if people really hate that, I'd write a simple dumb macro like

#define UseConstVersion(ret_T, this_T, method, args) \

    return const_cast<ret_T>(const_cast<const this_T*>(this)->method args)

// usage:

const WebFrame* WebViewImpl::MainFrame() const {

  UseConstVersion(WebFrame*, WebViewImpl, MainFrame, ());

}

PK

[Peter Kasting]

On Mon, Jun 5, 2017 at 3:18 PM, Peter Kasting <pkas...@chromium.org> wrote:

// usage:

const WebFrame* WebViewImpl::MainFrame() const {

  UseConstVersion(WebFrame*, WebViewImpl, MainFrame, ());

}

Sigh.   Pretend that method signature was the non-const version.

PK

[Matt Giuca]

Why not:

#define PROVIDE_NON_CONST_GETTER(ret_T, this_T, method, args) \
  ret_T this_T::method args { \

    return const_cast<ret_T>(const_cast<const this_T*>(this)->method args) \
  }

// usage:

PROVIDE_NON_CONST_GETTER(WebFrame*, WebViewImpl, MainFrame, ());

(Fits the same pattern as DISALLOW_COPY_AND_ASSIGN where you just chuck it in the class scope, no need to define a method body at all.)

I'd be happy with such a macro. Also happy to just use the idiom directly.

[Peter Kasting]

On Mon, Jun 5, 2017 at 5:41 PM, Matt Giuca <mgi...@chromium.org> wrote:

Why not:

#define PROVIDE_NON_CONST_GETTER(ret_T, this_T, method, args) \
  ret_T this_T::method args { \

    return const_cast<ret_T>(const_cast<const this_T*>(this)->method args) \
  }

// usage:

PROVIDE_NON_CONST_GETTER(WebFrame*, WebViewImpl, MainFrame, ());

That's fine too.  I don't prefer it, because I think it encourages people to use it more than they truly need to (I foresee a bunch of PROVIDE_NON_CONST_GETTERs proliferating in header files), but I don't care much.

Although if you want it used in a class declaration, I'd remove "this_T::".

PK 

[Jeremy Roman]

Personally, -1 to macros. I'm not sure this is much more readable. It ends up being even worse than this one because the method declaration needs to include the types, but the call should not.

I'm not convinced the macro adds much clarity over just writing out the const_cast, if we want to go that route.

[Brett Wilson]

On Mon, Jun 5, 2017 at 5:48 PM, Jeremy Roman <jbr...@chromium.org> wrote:

Personally, -1 to macros. I'm not sure this is much more readable. It ends up being even worse than this one because the method declaration needs to include the types, but the call should not.

I'm not convinced the macro adds much clarity over just writing out the const_cast, if we want to go that route.

+1, Macros make everything less clear.

Brett

[Alex Clarke]

Yeah macros would be ugly.  The other way of "solving" this is via templates and I don't think that's a good idea either.

Honestly I don't see const and non-const getters as being a problem worth solving, they're just part of the C++ baggage. 

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CACuR13cM7rf1Q7_EfEvCe%3D%3DaMtSEbHm2Np%2B15%3D%2BfPbTZYQ%2Bj0Q%40mail.gmail.com.

[Alex Clarke]

--

You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABiGVV_sFdj%3Diowf8jsmTHOi5WytmsX3CB%3D_mL80YDtXosTvRQ%40mail.gmail.com.

[Nick Carter]

If we want to make it easier for classes like Page to actually achieve a const-correct API that's logically const-correct, we should provide something in base/ along the lines of std::propagate_const.

The problem with const is that it stops reliably catching bugs the moment your class has std::unique_ptr or bare pointer members. For example, different 'const' rules apply to |foo_| and |bar_| below, although they're essentially equivalent ways of expressing composition:

class Example {
 T foo_;
 std::unique_ptr<T> bar_;

 void foo() const { foo_.mutate(); }     // compiler error
 void bar() const { bar_->mutate(); }   // compiles just fine

}

Consequently, the guarantee of 'const' provided by the compiler is very weak in your typical Chrome class, which is decomposed into smaller components, held as unique_ptrs or maybe a std::map of unique_ptrs. The original example of Page (which uses Member<>, which also doesn't propagate constness) represents an extreme but not uncommon case, where the compiler would allow just about any method could be marked const, even if it trivially leads to mutation of what's logically part of the class's state.

In such an environment, one cannot trust a 'const'-decorated method to be actually logically const, because it could denote either of the following:

  - a logically non-const method that can technically be physically const (because of pointer indirection), so it's marked const.
  - a logically const method that's part of an interface that's intended to be const correct.

With a class like Page, one needs to decide whether to (a) have const & non-const method pairs that are unnecessary to satisfy the C++ type system (these are at risk of being replaced with a single const method returning a non-const ref, by the next person along, particularly if their bodies are nontrivial or use an ugly const_cast), or (b) to drop const methods altogether (which limits the ability to play nice with adjacent classes that are trying to expose a const API, due to the virality of const), or (c) provide const methods that are logically const-incorrect, and don't actually find bugs. All these alternatives are unsatisfying in one way or another, but (b) is the most stable equilibrium. Even with an amended style guide that prefers (a), I fear we'll spend time adding and deleting const, cranking a wheel worthlessly, with const not actually doing its job and catching calls to bar_->mutate() in method bodies.

A template like std::propagate_const might provide a way out, though I have no personal experience with it. Maybe someone else does? If we could use something like that, I'd be much more supportive of exposing const-correct APIs from classes. Without it, I'm in the const-hostile, why-bother camp.

On Tue, Jun 6, 2017 at 11:20 AM, 'Alex Clarke' via platform-architecture-dev <platform-arc...@chromium.org> wrote:

On 6 June 2017 at 05:43, Brett Wilson <bre...@chromium.org> wrote:

On Mon, Jun 5, 2017 at 5:48 PM, Jeremy Roman <jbr...@chromium.org> wrote:

Personally, -1 to macros. I'm not sure this is much more readable. It ends up being even worse than this one because the method declaration needs to include the types, but the call should not.

I'm not convinced the macro adds much clarity over just writing out the const_cast, if we want to go that route.

+1, Macros make everything less clear.

Brett

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

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

To post to this group, send email to platform-architecture-dev@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABiGVV_sFdj%3Diowf8jsmTHOi5WytmsX3CB%3D_mL80YDtXosTvRQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

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

[Peter Kasting]

On Wed, Jun 7, 2017 at 2:20 PM, Nick Carter <ni...@chromium.org> wrote:

A template like std::propagate_const might provide a way out, though I have no personal experience with it. Maybe someone else does? If we could use something like that, I'd be much more supportive of exposing const-correct APIs from classes. Without it, I'm in the const-hostile, why-bother camp.

I would boil down your mail to basically:

"C++ as a language is broken because we want logical constness, the compiler enforces physical constness, and the two aren't the same.  A wrapper like this would be one way to annotate the code to try to get the compiler to enforce something more like logical constness.  If the compiler isn't enforcing this, it seems unlikely const correctness can be maintained, and I'd prefer to rip out all const."

Does that capture things accurately?

I agree with you that toolchain enforcement is better than author/reviewer enforcement.  I also agree that a solution like this can help make the toolchain enforce this more.  I am not certain that the pain of manually annotating all pointer members in all classes is worth it.  I definitely disagree that without this we should be hostile to const (and I think the style guide is with me).

The source of my final disagreement is disagreement with your analysis of the equilibrium here.

• While it's possible for people to remove const accessors, it's rare, even in today's world.
• It will be even less common when we document this in the Dos And Donts guide (just haven't gotten there yet)
• In cases where callers have const T objects, const accessor methods on T already can't be converted from (a) to (b) anyway.  The virality of const prevents easily ripping it back out, reducing the amount of wheel-spinning.  Basically, correct usage of const is not fully self-reinforcing, but it is somewhat self-reinforcing.

PK

[Sylvain Defresne]

I'm a bit late to the party, but I have difficulty with your three classes of const correctness.

In which category falls the "const" factory methods (for example KeyedServiceFactory::BuildServiceInstanceFor). It returns a new object, and the caller is owning it and is allowed to modify it, so it must be returned non-const. But the factory itself must not be mutated by this call, so the method is const.

It looks like according to your categories, this is "const broken", but I personally think this pattern is fine.

Another example would be std::unique_ptr<T>::get(). The method is const, but returns a non-const T* so that a "const std::unique_ptr<T>" behaves the same way as a "T* const" (likewise "std::unique_ptr<const T>" behaves like "const T*" and "const std::unique_ptr<const T>" for "const T* const").

-- Sylvain

On Mon, May 22, 2017 at 7:53 AM, Matt Giuca <mgi...@chromium.org> wrote:

I think we need to distinguish between three classes of correctness here (I'm not sure what the correct terminology is):

• "Const correct": More or less everything is const if it doesn't have to be modified. Methods that don't modify an object are const. This is "ideal" but it's totally arguable whether the benefits outweigh the cost of refactoring.
• "Const okay" (for lack of a better term): There are places where const could be used, but it isn't. But when const is used, it makes sense. Some methods are not const even though they don't modify the object. Some variables are not const even though they could be. But when a method does call itself "const", it respects that declaration. It doesn't modify its object. It doesn't const-cast, and it doesn't make semantic modifications that technically satisfy the compiler (e.g., a const method modifying an object through an owned non-const pointer).
• "Const broken": Methods declare themselves as "const", but they modify the object anyway, or return objects that can later be used to modify the object.

In the case of getters, you have:

• Non-const getter returns non-const object; const getter returns const object: This is "const correct".
• Non-const getter returns non-const object only: This is "const okay"; you need a non-const ref of an object just to read it, which means clients need to unnecessarily ask for write access. This is bad, but nobody is lying.
• Const getter returns non-const object: This is "const broken". The interface is a lie; the client does not need to ask for write access to an object, but they can modify its state anyway.

It sounds like Blink is currently in a "const okay" state. This particular class is "const correct". If you want to change it to "const okay" for consistency (i.e., remove the const getter), then that is fine, but your CL is proposing to make it "const broken".

Kentaro: When you say "Const-correctness is already seriously broken in Blink" -- do you mean Blink code is commonly "const broken" by my definition above, or merely "const okay"?

The fact that there are lots of clients that access those getters from a const object prevents you from simply removing the const getter. That suggests that at least some parts of Blink are operating in a const correct way. Const correctness is not all-or-nothing. I would suggest a useful goal to aim for is "const okay" everywhere, and "const correct" as much as possible, i.e., aim for it in new code but don't make a big effort to refactor old code around it. If you are writing new code that can't be const correct because of old code, don't bother to make the new code const correct, but avoid being "const broken" because that's confusing for everyone.

On Mon, 22 May 2017 at 15:04 Kentaro Hara <har...@chromium.org> wrote:

On Mon, May 22, 2017 at 1:03 PM, Sasha Bermeister <sas...@chromium.org> wrote:

Reading through the responses, there are clearly good arguments for both sides. Writing const-correct code would undoubtedly give us useful compile-time errors that ensure the code is behaving as expected, but in reality much of blink isn't currently written in a const-correct way and it would be an enormous (and likely non-trivial) effort to refactor the rest of the codebase to do this.

So the question becomes... Do we want to aim for a const-correct codebase, adding PRESUBMIT checks and style guide sections to explain the new requirements (e.g. every mutable field needs 2 getters, one const one non-const)? This would also mean we would eventually need a project to backport the rest of the codebase to be const correct. Also, many classes would not be able to be written const-correctly because of the legacy code they interact with, as explained above.

The alternative is to aim for consistency, making getters return const objects where they are not modified and non-const where they are, potentially adding const paths to non-const code (but keeping the code simple and straightforward).

I'd prefer this route.

Const-correctness is already seriously broken in Blink and I'm not sure if it's worth an effort to fix them.

 

I don't mind either, but leaving Page.h as the confusing outlier feels wrong unless we have a plan forward.

S

On Tue, May 16, 2017 at 1:40 PM, Matt Giuca <mgi...@chromium.org> wrote:

+1 to what Peter has said so far.

> I believe (without testing) you want "unique_ptr<const T>", which will vend you a const T* from get(), and which will block you from doing the sequence you described

I just tested this and it's correct.

You can't always guarantee perfectly the const correctness of things within an interface, but you should strive to have the public interface reflect conceptual const correctness. Consider:

class MyClass {

 public:

  // This is bad because a conceptually-const object is exposing a non-const

  // refernce to its internal field.

  int& x() const { return *x_; }

 private:

  // This is "fine" because only MyClass has access to the value. It's the code

  // inside MyClass's responsibility to *not* modify |*x_| from a const method.

  std::unique_ptr<int> x_;

};

This code compiles, but is not conceptually const-correct.

The ideal thing to do here would be to change |x_| to std::unique_ptr<const int>. Then the compiler checks const-correctness for you (the getter becomes a compile error and you realise that you need to make it return a const int*). However, that means |x_| has to always be const, not just from const methods. (You couldn't make a const and non-const getter.) So it's fine to keep |x_| as a non-const int, as long as you expose a const-correct interface:

  int& x() { return *x_; }

  const int& x() const { return *x_; }

Note: In my opinion, you should also strive to avoid non-const accessors wherever possible (the int& x() version). Even though it's const-correct, it violates encapsulation by exposing a member of MyClass for arbitrary modification by the client. MyClass has no way to control the value of its own variables. This isn't a hard rule; there are cases where this is useful, but you should prefer setters and mutation methods over non-const getters. In the CL in question it's not really feasible to remove these, though (since they have so many existing clients). I would just keep Page.h as-is.

On Tue, 16 May 2017 at 10:57 Sasha Bermeister <sas...@chromium.org> wrote:

+c...@chromium.org for more input.

On Tue, May 16, 2017 at 10:43 AM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, May 15, 2017 at 5:18 PM, Daniel Cheng <dch...@chromium.org> wrote:

The fact of the matter is that today, Blink has many cross-references between DOM objects and many of these are exposed as const member functions that return non-const pointers/references. If we try to implement full const-correctness, we'll have to double up a lot of simple methods. Do we expect this to prevent bugs?

Your question boils down to "do we expect const-correctness to prevent bugs".  I would read the underlying question as "is const-correctness worth doing".  Google style in general seems to say yes; see https://google.github.io/styleguide/cppguide.html#Use_of_const .  See also Effective C++, item 3.

The question of whether to retrofit "good practice" into a large existing codebase -- whether the effort pays the opportunity cost, and does not introduce new bugs -- is more complicated.  As someone who's not a core OWNER of Blink, that's not my decision to make.  However, many years ago, when I was even more gung-ho about "fix ALL the things", I did specifically recommend doing it to the core DOM classes in WebKit.  :)

Also, what's the difference between this and things like std::unique_ptr::get()/WTF::RefPtr::Get(), which are const but return a mutable pointer?

I don't know, as I wasn't part of the standardization discussion.  Note, however, that a "const unique_ptr<T>" is akin to a "T* const" and not a "const T*", so if you're thinking of unique_ptr analogously to a pointer -- which is, I think, the intent -- you don't really have a "pointer to const object" when you declare your unique_ptr const.  For that, I believe (without testing) you want "unique_ptr<const T>", which will vend you a const T* from get(), and which will block you from doing the sequence you described, as long as Next()/Prev()/Mutate() are const-correct.

PK

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFAnS6d9MZa5cqJACV3r1h2X-1%3DQg92CHCnWBBDOQ7xD3g%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

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

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CADAgNuo6bep%2BhbHUcS2Z8WAt1ko9%3DYnwYUGmL71vKrsJo0aGGw%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

--
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 post to this group, send email to c...@chromium.org.

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

[Peter Kasting]

On Thu, Jun 8, 2017 at 4:34 AM, Sylvain Defresne <sdef...@chromium.org> wrote:

I'm a bit late to the party, but I have difficulty with your three classes of const correctness.

In which category falls the "const" factory methods (for example KeyedServiceFactory::BuildServiceInstanceFor). It returns a new object, and the caller is owning it and is allowed to modify it, so it must be returned non-const. But the factory itself must not be mutated by this call, so the method is const.

It looks like according to your categories, this is "const broken", but I personally think this pattern is fine.

I haven't thought deeply enough about this case to argue strongly.  Note that my heuristic is intended to be an occasionally-overconservative guarantee.  There are cases where you can actually guarantee that the pointer you vend can never be used to transitively affect the state of the current object, so it's safe to write an API like this.

I suspect that it's not truly safe in this case and this really is "const-broken".  It would be interesting to know what making these factory methods non-const would break practically.  But not interesting enough for me to go spelunk :)

What I'm trying to say is, if someone assumes this usage is OK, I think the burden of proof is on them to demonstrate that it's impossible for anything to go wrong.

 

Another example would be std::unique_ptr<T>::get(). The method is const, but returns a non-const T* so that a "const std::unique_ptr<T>" behaves the same way as a "T* const" (likewise "std::unique_ptr<const T>" behaves like "const T*" and "const std::unique_ptr<const T>" for "const T* const").

As you note, the API here is trying to mimic how raw pointers behave, so const usage is designed to match the physical constness of raw pointer members.  It wouldn't really make sense for unique_ptr to have different const behavior than raw pointers if it was intended to be a drop-in replacement for them, so the library authors had no other choice.

But it does lead to the problem noted earlier where both raw and smart pointer members aren't "const-checked" by the compiler without something like propagate_const.  If the API here had been designed "const-correctly", then classes using smart pointer members would also have been forced to use them "const-correctly".  Harder to migrate to, but safer once you do.

PK

[Matt Giuca]

I think there's confusion between my 3 classes of const (as defined in an email I sent on May 22) and Peter's later rule of thumb (email on May 24) that a const method not return a non-const pointer.

There's a nuanced concept of "logical constness" which basically boils down to: if you have a pointer to an object that you *conceptually* own, then you should treat it as const if you are const. That is, don't return a non-const reference to an object you own, from a const getter. If you *don't* own the object, it *might* be safe to return non-const references, but there are non-obvious pitfalls involved (as discussed in detail above). Peter's rule of thumb deliberately over-reaches to avoid having to reason about those pitfalls.

As far as my definitions go, the only thing they are concerned about is: "const okay" means you don't make things const when they are logically non-const; "const correct" means in addition to that, you also don't make things non-const when they are logically const.

Having a const factory method that returns a non-const unique_ptr to a newly created object that doesn't hold any non-const pointers to the state inside the const object is probably "const correct", as I defined it, and probably perfectly safe. It represents an edge case in Peter's rule of thumb, but on the other hand, there's a possibility it would erroneously have a pointer into the logically-const factory so you still have to be careful.

[Peter Kasting]

I did my best to add guidance on const to the Dos and Donts page.

It's probably too long.  I wasn't sure how to shorten it and not risk future contention about why the recommendations there exist.

Patches welcome :)

PK

[Matt Giuca]

Thanks for adding this Peter. It is long! But it has a TL;DR and I think it's better to have a good detailed explanation than start further discussions.

The example you used there isn't actually correct:

T* T::GetMutationHandle() const { return this; }

In this case, "this" is actually a const T* so this is a compile error (as opposed to a logical const error that you're trying to show). Since this is Sites and not code-reviewable, I just edited it... replacing this example with the one you later touch on, about tree Nodes having left and right child. Actually a great illustration of the issue, since it looks harmless to have a const Tree return a non-const pointer to its left/right child. But this allows modification to the child which actually affects the entire tree.

--

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

To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFB74d%3DwpgyCjHH0xmmTfkZmVnnu06LOVE978TWnOKFGSQ%40mail.gmail.com.

[Peter Kasting]

On Wed, Jun 14, 2017 at 6:47 PM, Matt Giuca <mgi...@chromium.org> wrote:

The example you used there isn't actually correct:

T* T::GetMutationHandle() const { return this; }

In this case, "this" is actually a const T* so this is a compile error (as opposed to a logical const error that you're trying to show).

Ah, sigh.  That's what I get for trying to come up with the simplest possible examples off the top of my head :/

I could have const_cast to make this work, which would have helped illustrate the bit about const_casting too.  But that looks more obviously wrong to people, so it wouldn't have been a good example.

 

Since this is Sites and not code-reviewable, I just edited it... replacing this example with the one you later touch on, about tree Nodes having left and right child. Actually a great illustration of the issue, since it looks harmless to have a const Tree return a non-const pointer to its left/right child. But this allows modification to the child which actually affects the entire tree.

Thanks!  I further tweaked your example in hopes of making the problem even more obvious.

PK