--On Mon, May 15, 2017 at 5:18 PM, Daniel Cheng <dch...@chromium.org> wrote:The fact of the matter is that today, Blink has many cross-references between DOM objects and many of these are exposed as const member functions that return non-const pointers/references. If we try to implement full const-correctness, we'll have to double up a lot of simple methods. Do we expect this to prevent bugs?Your question boils down to "do we expect const-correctness to prevent bugs". I would read the underlying question as "is const-correctness worth doing". Google style in general seems to say yes; see https://google.github.io/styleguide/cppguide.html#Use_of_const . See also Effective C++, item 3.The question of whether to retrofit "good practice" into a large existing codebase -- whether the effort pays the opportunity cost, and does not introduce new bugs -- is more complicated. As someone who's not a core OWNER of Blink, that's not my decision to make. However, many years ago, when I was even more gung-ho about "fix ALL the things", I did specifically recommend doing it to the core DOM classes in WebKit. :)Also, what's the difference between this and things like std::unique_ptr::get()/WTF::RefPtr::Get(), which are const but return a mutable pointer?I don't know, as I wasn't part of the standardization discussion. Note, however, that a "const unique_ptr<T>" is akin to a "T* const" and not a "const T*", so if you're thinking of unique_ptr analogously to a pointer -- which is, I think, the intent -- you don't really have a "pointer to const object" when you declare your unique_ptr const. For that, I believe (without testing) you want "unique_ptr<const T>", which will vend you a const T* from get(), and which will block you from doing the sequence you described, as long as Next()/Prev()/Mutate() are const-correct.PK
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFAnS6d9MZa5cqJACV3r1h2X-1%3DQg92CHCnWBBDOQ7xD3g%40mail.gmail.com.
+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.
+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.
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).
+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 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 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.
+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.
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).
+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.
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: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."
- 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.
I think it's possible to make the codebase gradually const correct, if we allow to use const_cast on the const correctness boundary.
+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.
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-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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAHqYdcaHFeHtBWm9PzrhSFPhzeEyHrWPywB6W8No%3DzMMWmyuag%40mail.gmail.com.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.
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.
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.
- Is there any mechanical way to force the "const correct" rule?- Is there any mechanical way to force the "const okay" rule?
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.
Sgtm. Is there already guidance in the style guide or shall we add something?
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)."
"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.
--
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.
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/CAHqYdca0a-VpL%3D6W9ixAP2AUy2kfYu-_JiJxsLdcK1rvecRywg%40mail.gmail.com.
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.
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 havevoid 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.
https://chromium-review.googlesource.com/c/523925/2/third_party/WebKit/Source/web/WebViewImpl.cpp is one example.
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAJ_4DfTzVOkpYKTXPww4oEE77JnZ3emwLG3k06%3D1NW6wr%2B7GwA%40mail.gmail.com.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAF3XrKpjViLa0MMSUZhywbp15V2pAX7zvdezqYSeF4pxVesGwQ%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.
https://chromium-review.googlesource.com/c/523925/2/third_party/WebKit/Source/web/WebViewImpl.cpp is one example.
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFBNdo14kpqVcd8HHRnOAL1iEm%2BBx%2B_emeq9NCsfFgr%2B%2BQ%40mail.gmail.com.
Here's an ugly macro that works#define DEFINE_GETTERS(ReturnType, Method, Expr)\ReturnType Method() { \return (Expr);\}\\const ReturnType Method() const {\return (Expr);\}\
// usage:const WebFrame* WebViewImpl::MainFrame() const {UseConstVersion(WebFrame*, WebViewImpl, MainFrame, ());}
Why not:#define PROVIDE_NON_CONST_GETTER(ret_T, this_T, method, args) \
ret_T this_T::method args { \return const_cast<ret_T>(const_cast<const this_T*>(this)->method args) \
}// usage:PROVIDE_NON_CONST_GETTER(WebFrame*, WebViewImpl, MainFrame, ());
Personally, -1 to macros. I'm not sure this is much more readable. It ends up being even worse than this one because the method declaration needs to include the types, but the call should not.I'm not convinced the macro adds much clarity over just writing out the const_cast, if we want to go that route.
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CACuR13cM7rf1Q7_EfEvCe%3D%3DaMtSEbHm2Np%2B15%3D%2BfPbTZYQ%2Bj0Q%40mail.gmail.com.
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABiGVV_sFdj%3Diowf8jsmTHOi5WytmsX3CB%3D_mL80YDtXosTvRQ%40mail.gmail.com.
On 6 June 2017 at 05:43, Brett Wilson <bre...@chromium.org> wrote:
On Mon, Jun 5, 2017 at 5:48 PM, Jeremy Roman <jbr...@chromium.org> wrote:Personally, -1 to macros. I'm not sure this is much more readable. It ends up being even worse than this one because the method declaration needs to include the types, but the call should not.I'm not convinced the macro adds much clarity over just writing out the const_cast, if we want to go that route.+1, Macros make everything less clear.Brett
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsubsc...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABiGVV_sFdj%3Diowf8jsmTHOi5WytmsX3CB%3D_mL80YDtXosTvRQ%40mail.gmail.com.
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAPG_qM4yy1cBdp8JMncSy9frvKg8MAh0CNr41SVUV_2m-HF_MQ%40mail.gmail.com.
A template like std::propagate_const might provide a way out, though I have no personal experience with it. Maybe someone else does? If we could use something like that, I'd be much more supportive of exposing const-correct APIs from classes. Without it, I'm in the const-hostile, why-bother camp.
I think we need to distinguish between three classes of correctness here (I'm not sure what the correct terminology is):
- "Const correct": More or less everything is const if it doesn't have to be modified. Methods that don't modify an object are const. This is "ideal" but it's totally arguable whether the benefits outweigh the cost of refactoring.
- "Const okay" (for lack of a better term): There are places where const could be used, but it isn't. But when const is used, it makes sense. Some methods are not const even though they don't modify the object. Some variables are not const even though they could be. But when a method does call itself "const", it respects that declaration. It doesn't modify its object. It doesn't const-cast, and it doesn't make semantic modifications that technically satisfy the compiler (e.g., a const method modifying an object through an owned non-const pointer).
- "Const broken": Methods declare themselves as "const", but they modify the object anyway, or return objects that can later be used to modify the object.
In the case of getters, you have:
- Non-const getter returns non-const object; const getter returns const object: This is "const correct".
- Non-const getter returns non-const object only: This is "const okay"; you need a non-const ref of an object just to read it, which means clients need to unnecessarily ask for write access. This is bad, but nobody is lying.
- Const getter returns non-const object: This is "const broken". The interface is a lie; the client does not need to ask for write access to an object, but they can modify its state anyway.
It sounds like Blink is currently in a "const okay" state. This particular class is "const correct". If you want to change it to "const okay" for consistency (i.e., remove the const getter), then that is fine, but your CL is proposing to make it "const broken".Kentaro: When you say "Const-correctness is already seriously broken in Blink" -- do you mean Blink code is commonly "const broken" by my definition above, or merely "const okay"?The fact that there are lots of clients that access those getters from a const object prevents you from simply removing the const getter. That suggests that at least some parts of Blink are operating in a const correct way. Const correctness is not all-or-nothing. I would suggest a useful goal to aim for is "const okay" everywhere, and "const correct" as much as possible, i.e., aim for it in new code but don't make a big effort to refactor old code around it. If you are writing new code that can't be const correct because of old code, don't bother to make the new code const correct, but avoid being "const broken" because that's confusing for everyone.On Mon, 22 May 2017 at 15:04 Kentaro Hara <har...@chromium.org> wrote:On Mon, May 22, 2017 at 1:03 PM, Sasha Bermeister <sas...@chromium.org> wrote:Reading through the responses, there are clearly good arguments for both sides. Writing const-correct code would undoubtedly give us useful compile-time errors that ensure the code is behaving as expected, but in reality much of blink isn't currently written in a const-correct way and it would be an enormous (and likely non-trivial) effort to refactor the rest of the codebase to do this.So the question becomes... Do we want to aim for a const-correct codebase, adding PRESUBMIT checks and style guide sections to explain the new requirements (e.g. every mutable field needs 2 getters, one const one non-const)? This would also mean we would eventually need a project to backport the rest of the codebase to be const correct. Also, many classes would not be able to be written const-correctly because of the legacy code they interact with, as explained above.The alternative is to aim for consistency, making getters return const objects where they are not modified and non-const where they are, potentially adding const paths to non-const code (but keeping the code simple and straightforward).I'd prefer this route.Const-correctness is already seriously broken in Blink and I'm not sure if it's worth an effort to fix them.I don't mind either, but leaving Page.h as the confusing outlier feels wrong unless we have a plan forward.SOn Tue, May 16, 2017 at 1:40 PM, Matt Giuca <mgi...@chromium.org> wrote:+1 to what Peter has said so far.> I believe (without testing) you want "unique_ptr<const T>", which will vend you a const T* from get(), and which will block you from doing the sequence you describedI just tested this and it's correct.You can't always guarantee perfectly the const correctness of things within an interface, but you should strive to have the public interface reflect conceptual const correctness. Consider:class MyClass {public:// This is bad because a conceptually-const object is exposing a non-const// refernce to its internal field.int& x() const { return *x_; }private:// This is "fine" because only MyClass has access to the value. It's the code// inside MyClass's responsibility to *not* modify |*x_| from a const method.std::unique_ptr<int> x_;};This code compiles, but is not conceptually const-correct.The ideal thing to do here would be to change |x_| to std::unique_ptr<const int>. Then the compiler checks const-correctness for you (the getter becomes a compile error and you realise that you need to make it return a const int*). However, that means |x_| has to always be const, not just from const methods. (You couldn't make a const and non-const getter.) So it's fine to keep |x_| as a non-const int, as long as you expose a const-correct interface:int& x() { return *x_; }const int& x() const { return *x_; }Note: In my opinion, you should also strive to avoid non-const accessors wherever possible (the int& x() version). Even though it's const-correct, it violates encapsulation by exposing a member of MyClass for arbitrary modification by the client. MyClass has no way to control the value of its own variables. This isn't a hard rule; there are cases where this is useful, but you should prefer setters and mutation methods over non-const getters. In the CL in question it's not really feasible to remove these, though (since they have so many existing clients). I would just keep Page.h as-is.On Tue, 16 May 2017 at 10:57 Sasha Bermeister <sas...@chromium.org> wrote:+c...@chromium.org for more input.On Tue, May 16, 2017 at 10:43 AM, Peter Kasting <pkas...@chromium.org> wrote:On Mon, May 15, 2017 at 5:18 PM, Daniel Cheng <dch...@chromium.org> wrote:The fact of the matter is that today, Blink has many cross-references between DOM objects and many of these are exposed as const member functions that return non-const pointers/references. If we try to implement full const-correctness, we'll have to double up a lot of simple methods. Do we expect this to prevent bugs?Your question boils down to "do we expect const-correctness to prevent bugs". I would read the underlying question as "is const-correctness worth doing". Google style in general seems to say yes; see https://google.github.io/styleguide/cppguide.html#Use_of_const . See also Effective C++, item 3.The question of whether to retrofit "good practice" into a large existing codebase -- whether the effort pays the opportunity cost, and does not introduce new bugs -- is more complicated. As someone who's not a core OWNER of Blink, that's not my decision to make. However, many years ago, when I was even more gung-ho about "fix ALL the things", I did specifically recommend doing it to the core DOM classes in WebKit. :)Also, what's the difference between this and things like std::unique_ptr::get()/WTF::RefPtr::Get(), which are const but return a mutable pointer?I don't know, as I wasn't part of the standardization discussion. Note, however, that a "const unique_ptr<T>" is akin to a "T* const" and not a "const T*", so if you're thinking of unique_ptr analogously to a pointer -- which is, I think, the intent -- you don't really have a "pointer to const object" when you declare your unique_ptr const. For that, I believe (without testing) you want "unique_ptr<const T>", which will vend you a const T* from get(), and which will block you from doing the sequence you described, as long as Next()/Prev()/Mutate() are const-correct.PK
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFAnS6d9MZa5cqJACV3r1h2X-1%3DQg92CHCnWBBDOQ7xD3g%40mail.gmail.com.
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CADAgNuoffL8JYdtKx6qFVPxwXyeaoVcZG6ovdh0MAuG08m9Rew%40mail.gmail.com.
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CADAgNuo6bep%2BhbHUcS2Z8WAt1ko9%3DYnwYUGmL71vKrsJo0aGGw%40mail.gmail.com.
--Kentaro Hara, Tokyo, JapanTo view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHqYdcaHFeHtBWm9PzrhSFPhzeEyHrWPywB6W8No%3DzMMWmyuag%40mail.gmail.com.--
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.
I'm a bit late to the party, but I have difficulty with your three classes of const correctness.In which category falls the "const" factory methods (for example KeyedServiceFactory::BuildServiceInstanceFor). It returns a new object, and the caller is owning it and is allowed to modify it, so it must be returned non-const. But the factory itself must not be mutated by this call, so the method is const.It looks like according to your categories, this is "const broken", but I personally think this pattern is fine.
Another example would be std::unique_ptr<T>::get(). The method is const, but returns a non-const T* so that a "const std::unique_ptr<T>" behaves the same way as a "T* const" (likewise "std::unique_ptr<const T>" behaves like "const T*" and "const std::unique_ptr<const T>" for "const T* const").
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAAHOzFB74d%3DwpgyCjHH0xmmTfkZmVnnu06LOVE978TWnOKFGSQ%40mail.gmail.com.
The example you used there isn't actually correct:T* T::GetMutationHandle() const { return this; }In this case, "this" is actually a const T* so this is a compile error (as opposed to a logical const error that you're trying to show).
Since this is Sites and not code-reviewable, I just edited it... replacing this example with the one you later touch on, about tree Nodes having left and right child. Actually a great illustration of the issue, since it looks harmless to have a const Tree return a non-const pointer to its left/right child. But this allows modification to the child which actually affects the entire tree.