Thoughts on duplicate const/non-const getters

743 views
Skip to first unread message

Sasha Bermeister

unread,
May 15, 2017, 2:18:26 AM5/15/17
to platform-architecture-dev, Daniel Cheng, Stuart Langley, Jeremy Roman, Kentaro Hara, Dana Jansens, pkas...@chromium.org
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:

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

unread,
May 15, 2017, 3:10:06 AM5/15/17
to Sasha Bermeister, platform-architecture-dev, Daniel Cheng, Stuart Langley, Jeremy Roman, Dana Jansens, Peter Kasting
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:

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

unread,
May 15, 2017, 3:34:19 AM5/15/17
to Kentaro Hara, Sasha Bermeister, platform-architecture-dev, Daniel Cheng, Stuart Langley, Jeremy Roman, Dana Jansens, Peter Kasting
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:

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.



--
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.

Yuta Kitamura

unread,
May 15, 2017, 3:34:29 AM5/15/17
to Kentaro Hara, Sasha Bermeister, platform-architecture-dev, Daniel Cheng, Stuart Langley, Jeremy Roman, Dana Jansens, Peter Kasting
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:

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

unread,
May 15, 2017, 9:29:16 AM5/15/17
to Yuta Kitamura, Kentaro Hara, Sasha Bermeister, platform-architecture-dev, Daniel Cheng, Stuart Langley, Jeremy Roman, Dana Jansens, Peter Kasting
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.

Nico Weber

unread,
May 15, 2017, 11:14:56 AM5/15/17
to Dave Tapuska, Yuta Kitamura, Kentaro Hara, Sasha Bermeister, platform-architecture-dev, Daniel Cheng, Stuart Langley, Jeremy Roman, Dana Jansens, Peter Kasting
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.

--
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.

Peter Kasting

unread,
May 15, 2017, 12:51:26 PM5/15/17
to Yuta Kitamura, Kentaro Hara, Sasha Bermeister, platform-architecture-dev, Daniel Cheng, Stuart Langley, Jeremy Roman, Dana Jansens
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:

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

unread,
May 15, 2017, 8:18:32 PM5/15/17
to Peter Kasting, Yuta Kitamura, Kentaro Hara, Sasha Bermeister, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens
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

unread,
May 15, 2017, 8:43:59 PM5/15/17
to Daniel Cheng, Yuta Kitamura, Kentaro Hara, Sasha Bermeister, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens
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

unread,
May 15, 2017, 8:57:35 PM5/15/17
to Peter Kasting, Daniel Cheng, Yuta Kitamura, Kentaro Hara, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens, c...@chromium.org
+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

unread,
May 15, 2017, 11:40:24 PM5/15/17
to Sasha Bermeister, Peter Kasting, Daniel Cheng, Yuta Kitamura, Kentaro Hara, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens, c...@chromium.org
+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.

--
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

unread,
May 22, 2017, 12:04:06 AM5/22/17
to Matt Giuca, Peter Kasting, Daniel Cheng, Yuta Kitamura, Kentaro Hara, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens, c...@chromium.org, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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.

--
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

unread,
May 22, 2017, 1:04:30 AM5/22/17
to Sasha Bermeister, Matt Giuca, Peter Kasting, Daniel Cheng, Yuta Kitamura, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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.

--
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.

--
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

unread,
May 22, 2017, 1:53:36 AM5/22/17
to Kentaro Hara, Sasha Bermeister, Peter Kasting, Daniel Cheng, Yuta Kitamura, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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.

--
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.

--
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

unread,
May 22, 2017, 1:56:42 AM5/22/17
to Sasha Bermeister, Matt Giuca, Peter Kasting, Daniel Cheng, Kentaro Hara, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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.

--
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

unread,
May 22, 2017, 2:04:26 AM5/22/17
to Yuta Kitamura, Sasha Bermeister, Peter Kasting, Daniel Cheng, Kentaro Hara, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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.

--
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

unread,
May 22, 2017, 2:37:28 AM5/22/17
to Matt Giuca, Sasha Bermeister, Peter Kasting, Daniel Cheng, Yuta Kitamura, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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.

--
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.

--
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.

--
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

unread,
May 22, 2017, 2:56:59 AM5/22/17
to Kentaro Hara, Matt Giuca, Sasha Bermeister, Peter Kasting, Daniel Cheng, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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

unread,
May 22, 2017, 3:06:38 AM5/22/17
to Yuta Kitamura, Matt Giuca, Sasha Bermeister, Peter Kasting, Daniel Cheng, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
Yeah, so my suggestion is: let's not break the code more by forcing the "const okay" rule for new code :D



Yuta Kitamura

unread,
May 22, 2017, 3:38:24 AM5/22/17
to Matt Giuca, Sasha Bermeister, Peter Kasting, Daniel Cheng, Kentaro Hara, platform-architecture-dev, Stuart Langley, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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

unread,
May 22, 2017, 7:20:08 AM5/22/17
to Yuta Kitamura, Matt Giuca, Sasha Bermeister, Peter Kasting, Daniel Cheng, Kentaro Hara, platform-architecture-dev, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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

unread,
May 22, 2017, 8:02:05 AM5/22/17
to Stuart Langley, Yuta Kitamura, Matt Giuca, Sasha Bermeister, Peter Kasting, Daniel Cheng, platform-architecture-dev, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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

unread,
May 22, 2017, 1:08:17 PM5/22/17
to Kentaro Hara, Stuart Langley, Yuta Kitamura, Matt Giuca, Sasha Bermeister, Daniel Cheng, platform-architecture-dev, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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

unread,
May 22, 2017, 10:14:00 PM5/22/17
to Peter Kasting, Kentaro Hara, Stuart Langley, Yuta Kitamura, Sasha Bermeister, Daniel Cheng, platform-architecture-dev, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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

unread,
May 23, 2017, 8:02:32 PM5/23/17
to Matt Giuca, Peter Kasting, Stuart Langley, Yuta Kitamura, Sasha Bermeister, Daniel Cheng, platform-architecture-dev, Jeremy Roman, Dana Jansens, cxx, Dimitri Glazkov, bnu...@chromium.org, TAMURA, Kent
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

unread,
May 23, 2017, 8:11:31 PM5/23/17
to Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Peter Kasting, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Matt Giuca, Jeremy Roman
Sgtm. Is there already guidance in the style guide or shall we add something?

Peter Kasting

unread,
May 23, 2017, 8:14:30 PM5/23/17
to Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Matt Giuca, Jeremy Roman
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

unread,
May 23, 2017, 8:59:38 PM5/23/17
to Peter Kasting, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Matt Giuca, Jeremy Roman
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

unread,
May 23, 2017, 9:12:05 PM5/23/17
to Stuart Langley, Peter Kasting, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
May 23, 2017, 10:31:08 PM5/23/17
to Matt Giuca, Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
May 23, 2017, 10:37:33 PM5/23/17
to Matt Giuca, Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
May 23, 2017, 10:55:31 PM5/23/17
to Peter Kasting, Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
May 23, 2017, 11:11:31 PM5/23/17
to Peter Kasting, Matt Giuca, Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
May 23, 2017, 11:32:39 PM5/23/17
to Ryan Hamilton, Peter Kasting, Matt Giuca, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
May 23, 2017, 11:53:32 PM5/23/17
to Stuart Langley, Ryan Hamilton, Peter Kasting, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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());".

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

unread,
May 24, 2017, 2:51:03 AM5/24/17
to Ryan Hamilton, Matt Giuca, Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
May 24, 2017, 2:56:34 AM5/24/17
to Peter Kasting, Ryan Hamilton, Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
May 25, 2017, 5:37:43 AM5/25/17
to Matt Giuca, Peter Kasting, Ryan Hamilton, Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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.

Peter Kasting

unread,
May 25, 2017, 2:43:53 PM5/25/17
to Anthony Berent, Matt Giuca, Ryan Hamilton, Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
May 25, 2017, 4:37:34 PM5/25/17
to Peter Kasting, Matt Giuca, Ryan Hamilton, Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
May 25, 2017, 6:22:59 PM5/25/17
to Anthony Berent, Matt Giuca, Ryan Hamilton, Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
May 25, 2017, 9:29:40 PM5/25/17
to Peter Kasting, Anthony Berent, Ryan Hamilton, Stuart Langley, Kentaro Hara, Daniel Cheng, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
Jun 5, 2017, 4:57:52 PM6/5/17
to Matt Giuca, Peter Kasting, Anthony Berent, Ryan Hamilton, Stuart Langley, Kentaro Hara, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
Jun 5, 2017, 5:25:17 PM6/5/17
to Daniel Cheng, Matt Giuca, Peter Kasting, Anthony Berent, Stuart Langley, Kentaro Hara, TAMURA, Kent, Yuta Kitamura, Dimitri Glazkov, platform-architecture-dev, cxx, Sasha Bermeister, bnu...@chromium.org, Dana Jansens, Jeremy Roman
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

unread,
Jun 5, 2017, 5:34:10 PM6/5/17