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.