Re: [webkit-dev] Do we have a style preference about const member functions?
On Jun 8, 2011, at 11:48 AM, Peter Kasting wrote: On Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak m...@apple.com wrote: 1) We definitely have consensus to fix the broken non-logically-const accessors by making them non-const; consensus on also adding const accessors is less clear. There are a surprising number of places that actually do const traversals. Simply making all these accessors non-const will require removing a lot of valid const usage from the existing code. I'm really uncomfortable with that. 2) We like to do things incrementally and fixing bad use of const before adding good use of const seems like this is a logical way to split the work and make it less of a megapatch. Incremental fixes are absolutely the way to go. Reviewing megapatches sucks and it's hard to catch subtle bugs like you changed this function to be not const, but there's no reason to do that. Perhaps a split that avoids removing existing, valid const usage would be to first change few (or no) function signatures, and simply modify caller code to be more consistent about type declarations. This would mean converting some callers from Node* to const Node* when they're doing true const traversals, and some the opposite direction when they're not. The goal would be to make eventual API changes a no-op in terms of caller effect. It'd be easy to make these sorts of patches arbitrarily small as well. How about posting a reasonable-sized patch that handles a few related, representative methods so we can evaluate. (As a separate technical comment, I think it may be better to have the const versions call non-const versions (casting away const on this), because the non-const versions are likely to be called much more often so it's helpful to have fewer levels of indirection to wade through before seeing the real code, e.g. when inspecting code or debugging.) I totally agree that these sorts of indirections are suboptimal (especially for common cases). The particular idea you propose isn't safe because there's no protection against the non-const impl actually causing side effects. Even if current implementations don't, it's easy to add a subtle bug like this in the future. And while compilers won't enforce this perfectly, they'll do a better and better job (better than nothing, for sure) as we change more APIs to enforce logical constness. (I hate to keep referencing it, but the end of Effective C++ Item 3 directly addresses this implementation idea in more detail. That whole section is really worth reading for anyone deeply interested in this issue.) If the best way to add const to the DOM is to add a level of indirection to all the non-const methods, then I'd probably think that is a reason to drop const entirely. But I'm not really convinced that casting away const from a return value is intrinsically safer than casting away const from this. However, I think we should at least try to limit the number of accessor pairs to only those cases which really need them. What about this plan: * I'll post a patch that just addresses the caller-side constness issues I've found from my work so far. * Then in my local checkout I'll start trying to see which accessor pairs I can collapse back to one accessor, be it const or non-const. I'll post patches to make any necessary caller-side changes here, too. * Finally I'll post a patch to change method signatures and add accessor pairs where necessary. * Then rinse and repeat with another class, e.g. Element. I'll go ahead and file a bug and start posting real diffs to look at unless this plan has fatal flaws. Hard to say without seeing the patches but I'd be glad to look at anything you post. - Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Thu, Jun 9, 2011 at 2:49 AM, Maciej Stachowiak m...@apple.com wrote: I'm not really convinced that casting away const from a return value is intrinsically safer than casting away const from this. Allowing the caller to mutate the return value is fine because the caller had a non-const |this| to begin with. We're not making anything less const-safe. Casting away const on |this|, OTOH, allows you to mutate objects even when you never had permission to begin with. Much different. In any case, my intent is to proceed as Darin and I discussed. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Thu, Jun 9, 2011 at 3:51 PM, Maciej Stachowiak m...@apple.com wrote: In principle, the return value could have been retrieved from a container that the immediate callee only has a const reference to. So then casting away const on the return value would be a hazard. You're right; if the implementation of the const function accesses other objects besides |this|, it might be better to re-implement the non-const function rather than simply casting, so the compiler can at least check those accesses. This implies that in general we'd want to limit function pairs to accessors with fairly simple implementations, and only make exceptions where there's real value, lest the maintenance burden and bug surface outweigh any gain from providing caller const convenience. I'll try to keep this in mind before I try posting any patches that split accessors into pairs. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Jun 7, 2011, at 4:22 PM, Darin Adler wrote: I think the “make some things non-const” part of this would be the first part to put up for review and land. To expand on Darin's remark in two different ways: 1) We definitely have consensus to fix the broken non-logically-const accessors by making them non-const; consensus on also adding const accessors is less clear. 2) We like to do things incrementally and fixing bad use of const before adding good use of const seems like this is a logical way to split the work and make it less of a megapatch. (As a separate technical comment, I think it may be better to have the const versions call non-const versions (casting away const on this), because the non-const versions are likely to be called much more often so it's helpful to have fewer levels of indirection to wade through before seeing the real code, e.g. when inspecting code or debugging.) Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak m...@apple.com wrote: 1) We definitely have consensus to fix the broken non-logically-const accessors by making them non-const; consensus on also adding const accessors is less clear. There are a surprising number of places that actually do const traversals. Simply making all these accessors non-const will require removing a lot of valid const usage from the existing code. I'm really uncomfortable with that. 2) We like to do things incrementally and fixing bad use of const before adding good use of const seems like this is a logical way to split the work and make it less of a megapatch. Incremental fixes are absolutely the way to go. Reviewing megapatches sucks and it's hard to catch subtle bugs like you changed this function to be not const, but there's no reason to do that. Perhaps a split that avoids removing existing, valid const usage would be to first change few (or no) function signatures, and simply modify caller code to be more consistent about type declarations. This would mean converting some callers from Node* to const Node* when they're doing true const traversals, and some the opposite direction when they're not. The goal would be to make eventual API changes a no-op in terms of caller effect. It'd be easy to make these sorts of patches arbitrarily small as well. (As a separate technical comment, I think it may be better to have the const versions call non-const versions (casting away const on this), because the non-const versions are likely to be called much more often so it's helpful to have fewer levels of indirection to wade through before seeing the real code, e.g. when inspecting code or debugging.) I totally agree that these sorts of indirections are suboptimal (especially for common cases). The particular idea you propose isn't safe because there's no protection against the non-const impl actually causing side effects. Even if current implementations don't, it's easy to add a subtle bug like this in the future. And while compilers won't enforce this perfectly, they'll do a better and better job (better than nothing, for sure) as we change more APIs to enforce logical constness. (I hate to keep referencing it, but the end of Effective C++ Item 3 directly addresses this implementation idea in more detail. That whole section is really worth reading for anyone deeply interested in this issue.) However, I think we should at least try to limit the number of accessor pairs to only those cases which really need them. What about this plan: * I'll post a patch that just addresses the caller-side constness issues I've found from my work so far. * Then in my local checkout I'll start trying to see which accessor pairs I can collapse back to one accessor, be it const or non-const. I'll post patches to make any necessary caller-side changes here, too. * Finally I'll post a patch to change method signatures and add accessor pairs where necessary. * Then rinse and repeat with another class, e.g. Element. I'll go ahead and file a bug and start posting real diffs to look at unless this plan has fatal flaws. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Jun 8, 2011, at 11:48 AM, Peter Kasting wrote: On Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak m...@apple.com wrote: 1) We definitely have consensus to fix the broken non-logically-const accessors by making them non-const; consensus on also adding const accessors is less clear. There are a surprising number of places that actually do const traversals. Simply making all these accessors non-const will require removing a lot of valid const usage from the existing code. I'm really uncomfortable with that. I thought you did it already locally. You mentioned that you decided for many member functions that the right thing was to remove const. I suggested you land those changes first, before making the other changes. Are we talking about the same thing? Maybe you think Maciej is asking for something he’s not. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Wed, Jun 8, 2011 at 11:51 AM, Darin Adler da...@apple.com wrote: On Jun 8, 2011, at 11:48 AM, Peter Kasting wrote: On Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak m...@apple.com wrote: 1) We definitely have consensus to fix the broken non-logically-const accessors by making them non-const; consensus on also adding const accessors is less clear. There are a surprising number of places that actually do const traversals. Simply making all these accessors non-const will require removing a lot of valid const usage from the existing code. I'm really uncomfortable with that. I thought you did it already locally. You mentioned that you decided for many member functions that the right thing was to remove const. I suggested you land those changes first, before making the other changes. Are we talking about the same thing? Maybe you think Maciej is asking for something he’s not. Maybe I got confused. Some accessors cannot be const at all (IMO), like the ones that update layout before returning the desired value. Other accessors, e.g. parentNode(), don't themselves do anything non-const and so they could theoretically be valid as const and non-const versions. What I thought Maciej was saying was that we should remove const on all the existing accessors, in both categories, which sounded different than what you were saying (which I read as remove const on the accessors in the first category). I'm perfectly happy removing const from accessors in the first category. I can post a change that does that before going any further. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Jun 8, 2011, at 11:56 AM, Peter Kasting wrote: What I thought Maciej was saying was that we should remove const on all the existing accessors, in both categories, which sounded different than what you were saying (which I read as remove const on the accessors in the first category). I'm perfectly happy removing const from accessors in the first category. I can post a change that does that before going any further. Yes, I believe that’s what both Maciej and I were suggesting. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Wed, Jun 8, 2011 at 11:59 AM, Darin Adler da...@apple.com wrote: On Jun 8, 2011, at 11:56 AM, Peter Kasting wrote: I'm perfectly happy removing const from accessors in the first category. I can post a change that does that before going any further. Yes, I believe that’s what both Maciej and I were suggesting. Great. I filed https://bugs.webkit.org/show_bug.cgi?id=62302 about this and will proceed there. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Fri, Jun 3, 2011 at 5:59 PM, Darin Adler da...@apple.com wrote: On Jun 3, 2011, at 5:46 PM, Peter Kasting wrote: From the perspective of Node itself, I'm not sure why this would be a big task. There shouldn't be any const accessors that return non-const pointers. Simply removing the const qualifier on all the above accessors would make things correct, at the expense of possibly making it difficult to use a const Node*. Maybe you can give it a try and report back. I think you’ll find that this is an enormous task. I now have a local patch which mostly fixes const-correctness of the API declared in Node.h, which I've tested only for Chromium Windows (and only WebCore, no other bits). Before I post it for review somewhere I'd like to get feedback on how to proceed. By fixes, I mean that in most cases, I converted any A* Node::getA() const accessor to a pair of accessors, const A* Node::getA() const and A* Node::getA(). This should flush out people who are trying to perform non-const operations on const pointers, without preventing existing valid usage of const pointers, at the cost of adding a lot of extra accessors that aren't necessarily called. It doesn't guarantee transitive const correctness on other classes besides Node which may have similar accessors, but that's something of an advantage here because it allows me to write a patch to improve Node without having to fix all accessors across all WebCore. For all these pairs of accessors I used the trick from Effective C++ of having the non-const version call the const version and then cast away const on the return type, which looks a little unwieldy at first but guarantees the two accessors cannot get out of step in the future. I avoided fixing Node::scriptExecutionContext() because that's overridden in a zillion different places and I didn't want to touch all of those. I did fix a number of other accessors in other files where they were transitively affected by the changes above. Finally, I made a few decisions on functions to simply make non-const. For example, any accessor which called Document::updateLayoutIgnorePendingStylesheets() was made non-const because that really does have a lot of side-effects. According to jamesr this may be a good thing anyway because apparently we've had bugs (including security bugs) in the past where people call some innocuous-looking const accessor like Node::isContentEditable() and then they end up causing unintentional changes. I'm happy to post this for review somewhere. However, I wonder if first I should try to reduce the pairs of accessors I created above back down to one accessor wherever possible, based on which version is actually called. This would reduce API sprawl at the cost of making the actual APIs somewhat arbitrarily const or not. This might also discourage future users of these classes from re-adding the partner of an existing accessor, in the same way that writing classes without any const accessors encourages people not to ever try to use them via const pointers. Assuming we want this, I would also like to know how we'd like to proceed after this patch. I'm happy to clean up major classes like Element and Document one at a time, but it'd be nice if we settled on a consistent style policy and, if we want to fix a lot of the existing code, if more people than me could help (or if we could automate some things with tooling). PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Jun 2, 2011, at 1:32 AM, Ryosuke Niwa wrote: All functions passed to enclosingNodeOfType in htmlediting.cpp are such clients: Node* enclosingNodeOfType(const Position p, bool (*nodeIsOfType)(const Node*), EditingBoundaryCrossingRule rule) It takes a boolean function that takes const pointer to a DOM node. It is critical that nodeIsOfType does not modify DOM This points to a place where const does not work well. Having the single node pointer that is the argument to the function be const does not express “must not modify DOM”. If there was some way to express “must not modify DOM” that would be great, but that just expresses “must not modify this DOM node”. It happens that the predicate takes a node argument. You could imagine a similar function that takes a Range. You can see that that’s an even clearer. There’s no way to pass a range to a function and also say “must not modify DOM”. I don’t think const is a good way to express this. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Jun 3, 2011, at 7:50 AM, Darin Adler wrote: On Jun 2, 2011, at 1:32 AM, Ryosuke Niwa wrote: All functions passed to enclosingNodeOfType in htmlediting.cpp are such clients: Node* enclosingNodeOfType(const Position p, bool (*nodeIsOfType)(const Node*), EditingBoundaryCrossingRule rule) It takes a boolean function that takes const pointer to a DOM node. It is critical that nodeIsOfType does not modify DOM This points to a place where const does not work well. Having the single node pointer that is the argument to the function be const does not express “must not modify DOM”. If there was some way to express “must not modify DOM” that would be great, but that just expresses “must not modify this DOM node”. It happens that the predicate takes a node argument. You could imagine a similar function that takes a Range. You can see that that’s an even clearer. There’s no way to pass a range to a function and also say “must not modify DOM”. I don’t think const is a good way to express this. Are you referring to the fact that from a const Node* can be used to get non-const pointers to the Node's neighbors in the DOM, or something else? Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Jun 3, 2011, at 9:28 AM, Maciej Stachowiak wrote: On Jun 3, 2011, at 7:50 AM, Darin Adler wrote: On Jun 2, 2011, at 1:32 AM, Ryosuke Niwa wrote: All functions passed to enclosingNodeOfType in htmlediting.cpp are such clients: Node* enclosingNodeOfType(const Position p, bool (*nodeIsOfType)(const Node*), EditingBoundaryCrossingRule rule) It takes a boolean function that takes const pointer to a DOM node. It is critical that nodeIsOfType does not modify DOM This points to a place where const does not work well. Having the single node pointer that is the argument to the function be const does not express “must not modify DOM”. If there was some way to express “must not modify DOM” that would be great, but that just expresses “must not modify this DOM node”. It happens that the predicate takes a node argument. You could imagine a similar function that takes a Range. You can see that that’s an even clearer. There’s no way to pass a range to a function and also say “must not modify DOM”. I don’t think const is a good way to express this. Are you referring to the fact that from a const Node* can be used to get non-const pointers to the Node's neighbors in the DOM, or something else? Sort of. From a const Node* you can get: - a non-const pointer to a parent, sibling, or child - a non-const pointer to the document - a non-const pointer to the renderer - a non-const pointer to the style - a non-const pointer to various shadow-related ancestors and hosts That’s one problem. Extending the const-ness of the node to mean constness of everything you can get through all these pointers would be a big task, and it’s not clear it would be worthwhile. Further, from the document you can get to the frame and things like the selection controller. Experience with the C++ standard library taught me that a constant pointer to something within a collection is a difficult concept to model with const. The key example here is the std::vector::erase function. It seems illogical that you can’t call erase on a const_iterator, because the iterator is identifying the location within the vector, not whether you have permission to modify the vector. You’re not modifying something through the iterator. In the same sense, it seems to me that if there is such a thing as a const Node* I should be able to use it to set the selection in a document as long as I have a non-const pointer to that document. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Fri, Jun 3, 2011 at 5:27 PM, Darin Adler da...@apple.com wrote: From a const Node* you can get: - a non-const pointer to a parent, sibling, or child - a non-const pointer to the document - a non-const pointer to the renderer - a non-const pointer to the style - a non-const pointer to various shadow-related ancestors and hosts That’s one problem. Extending the const-ness of the node to mean constness of everything you can get through all these pointers would be a big task, and it’s not clear it would be worthwhile. From the perspective of Node itself, I'm not sure why this would be a big task. There shouldn't be any const accessors that return non-const pointers. Simply removing the const qualifier on all the above accessors would make things correct, at the expense of possibly making it difficult to use a const Node*. Adding const versions of the accessors where necessary, which themselves vend const pointers, is not significantly harder. The only way this is a big task is if there are callers that make significant use of const pointers to do non-const actions. Then we need to clean these up. But if they're doing non-const actions, then the cleanup is simply to make them use non-const pointers. Further, from the document you can get to the frame and things like the selection controller. Similarly, you shouldn't be able to get non-const pointers to the frame or selection controller from a const document pointer. Experience with the C++ standard library taught me that a constant pointer to something within a collection is a difficult concept to model with const. The key example here is the std::vector::erase function. It seems illogical that you can’t call erase on a const_iterator, because the iterator is identifying the location within the vector, not whether you have permission to modify the vector. You’re not modifying something through the iterator. Whether or not it's reasonable to have non-const functions on containers, which modify only the container and not the elements within it, take const pointers to elements, seems like a separable question from the rest of the issues with using const correctly. I happen to find the standard library's behavior here more reasonable than what you suggest, but regardless, all of the other cases above could certainly be handled in a clearly correct fashion. In other words, I don't find this problem a compelling reason to discard the entire idea of constness when it comes to objects which participate in tree/graph relationships. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Jun 3, 2011, at 5:46 PM, Peter Kasting wrote: From the perspective of Node itself, I'm not sure why this would be a big task. There shouldn't be any const accessors that return non-const pointers. Simply removing the const qualifier on all the above accessors would make things correct, at the expense of possibly making it difficult to use a const Node*. Maybe you can give it a try and report back. I think you’ll find that this is an enormous task. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Jun 1, 2011, at 8:11 PM, James Robinson wrote: On Tue, May 31, 2011 at 12:08 PM, Geoffrey Garen gga...@apple.com wrote: I agree that const should be used for logical constness. The rule should not be merely doesn't alter any data members of this object but rather does not alter observable state of this object or vend any type of pointer or reference by which observable state of this object could be altered. Precisely! I like this explanation too. I'm trying to think of a simple way to explain / test whether something falls into the category of logical constness, since it can be ambiguous. It occurred to me that a simple, though imperfect, test is just, Is this function called by an owner of a const pointer / reference? After all, a const member function is meaningless if nobody points to the class through a const pointer / reference. For classes like DOM and render tree nodes, which have no meaningful const-pointer-owning clients, the answer is always no. For other classes, the answer is yes, but only if someone has found a meaningful use for a const pointer or reference to the class. So, perhaps the real style question we should answer is when to use const pointers / references, since the answer to when to use const member functions will follow from it. What are some good examples of existing code that meaningfully uses a const pointer or reference? (Something other than, say, the obligatory const in a copy constructor.) I personally find that in a number of render tree functions declared to take a const RenderObject* that the const-ness is a useful hint that the function will not be manipulating the object in unexpected ways. Examples: http://codesearch.google.com/codesearch?hl=envert=chromiumlr=q=%22const+RenderObject%22+file%3AWebCoresbtn=Search (click Next at the bottom to see more results). My understanding is that these functions could not take a const RenderObject* parameter if RenderObject did not supply a set of const simple accessors and utility functions. We could convert RenderObject over to have no const member functions and convert all of these utility functions to take RenderObject* parameters instead, but I think that would lose some of the intent of the code. I'm curious if there was a specific patch or piece of code that lead you to send this email out - perhaps we make a better decision about whether to change our approach with more concrete examples of problems the current situation has caused or is causing. Looks to me like these fulfill the requirement of do we ever use const pointers to these objects. So the next step is to fix up const member functions that inappropriately return non-const pointers to objects in the same tree (or that can otherwise get a back pointer). Make the functions non-const, then add const variants returning a const pointer if needed. I'm curious if there are any clients for const pointers to DOM nodes. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Tue, May 31, 2011 at 12:08 PM, Geoffrey Garen gga...@apple.com wrote: So, perhaps the real style question we should answer is when to use const pointers / references, since the answer to when to use const member functions will follow from it. What are some good examples of existing code that meaningfully uses a const pointer or reference? (Something other than, say, the obligatory const in a copy constructor.) I think const reference to String and const pointers to CSSStyleDeclaration/CSSMutableStyleDeclaration are good examples. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
I think we should add a rule like a const member function should never return non-const pointer/reference. I have seen the problem in many places that you can get non-const reference on a const object. For example, InlineTextBox has InlineTextBox* prevTextBox() const; InlineTextBox* nextTextBox() const; Assume you have a function: void myFunction(const InlineTextBox* textBox) When the text box has a sibling, you can easily get a non-const pointer by calling textBox-nextTextBox()-prevTextBox(), which sounds ridiculous. 2011/5/31 Geoffrey Garen gga...@apple.com: Even in a class that is used in a tree, I still think simple member variable accessor methods (that do not return tree neighbors) should be const. OK. Why? Because it indicates to me and the compiler, that the method doesn't have side effects. A const member function can have side effects. It can modify any global state outside the object. It can also modify sub-objects inside the object, and return non-const references to sub-objects and related objects that might be used to produce side-effects at any time. It's exactly statements like this that make me think that const member functions are a bad idea -- people think they provide a guarantee, but they don't. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
Similar thing: doc-frame()-document().
The solution should be defining both const and non-const members:
const Frame* frame() const { reutrn m_frame; }
Frame* frame() { return m_frame; }
2011/6/2 Yong Li yong.li.web...@gmail.com:
I think we should add a rule like a const member function should
never return non-const pointer/reference.
I have seen the problem in many places that you can get non-const
reference on a const object.
For example, InlineTextBox has
InlineTextBox* prevTextBox() const;
InlineTextBox* nextTextBox() const;
Assume you have a function: void myFunction(const InlineTextBox* textBox)
When the text box has a sibling, you can easily get a non-const
pointer by calling textBox-nextTextBox()-prevTextBox(), which sounds
ridiculous.
2011/5/31 Geoffrey Garen gga...@apple.com:
Even in a class that is used in a tree, I still think simple member
variable accessor methods (that do not return tree neighbors) should be
const.
OK. Why?
Because it indicates to me and the compiler, that the method doesn't have
side effects.
A const member function can have side effects. It can modify any global
state outside the object. It can also modify sub-objects inside the object,
and return non-const references to sub-objects and related objects that
might be used to produce side-effects at any time.
It's exactly statements like this that make me think that const member
functions are a bad idea -- people think they provide a guarantee, but they
don't.
Geoff
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
Responding to a few comments at once: I'm curious if there was a specific patch or piece of code that lead you to send this email out - perhaps we make a better decision about whether to change our approach with more concrete examples of problems the current situation has caused or is causing. In https://bugs.webkit.org/show_bug.cgi?id=59604, Simon made a general comment that new methods should use const, but Alexey disagreed. Then I began this discussion, in the hopes of resolving the issue and writing a clear style guideline. I agree that concrete code is easier to discuss than abstract principles. Still, I'd like to write a general style guideline that coders and reviewers can use. On the specific question of accessors, it sounds like there's some consensus that accessors should be const, unless they return pointers or references, in which case accessors should be non-const, or should return const pointers or const references. In some cases, this may require two copies of the same accessor, but we'd rather not provide two copies of every accessor. Is that a fair summary? What are some good examples of existing code that meaningfully uses a const pointer or reference? (Something other than, say, the obligatory const in a copy constructor.) I personally find that in a number of render tree functions declared to take a const RenderObject* that the const-ness is a useful hint that the function will not be manipulating the object in unexpected ways. Examples: http://codesearch.google.com/codesearch?hl=envert=chromiumlr=q=%22const+RenderObject%22+file%3AWebCoresbtn=Search (click Next at the bottom to see more results). I think const reference to String and const pointers to CSSStyleDeclaration/CSSMutableStyleDeclaration are good examples. All functions passed to enclosingNodeOfType in htmlediting.cpp are such clients: Thanks, this is a good set of examples. Perhaps we could at least encourage const-correctness for newly-written classes? By this I mean both adherence to the logical-constness rules you stated earlier as well as not adding non-const accessors and members unless needed -- otherwise it's easy to just err on the side of never using const anywhere. I'm still not convinced that it's meaningful to talk about a const-correct class. The const that you put on a member function only has meaning when somebody uses a const pointer or reference to your class. More importantly, any design issues raised or bugs caught by const only get tested when somebody uses a const pointer or reference to your class. So, the key to const-correctness is deciding when to use const pointers and const references -- once you're using a const pointer or reference, the compiler will tell you which member functions need to be const. I also think we should not discourage people from using const on variables. I've gotten review comments in the past that have asked me to remove const qualifiers on locals, as well as sometimes on arguments and members, and I think this is a mistake. I tend to agree. On a project-wide scale, it's an obvious contradiction, and a bit three-stooges-esque, for reviewers to ask for more const member function declarations and fewer const variable declarations. There is no const-correctness without const variable declarations. That said, I think const is most useful when used with a pointer or a reference, because it provides for a separation of concerns, and acts as a barrier between logically independent pieces of code that live in different source files but share data. Perhaps a data member also falls into this category, since it can be modified by any member function. In contrast, declaring a local int const will only protect you from contradicting your own intentions inside one block scope -- to me, that's not really worth the extra verbosity. However, in the interests of pragmatism I think that it would be reasonable to at least remove the improper uses of const (b). OK, it seems that there is also some consensus toward removing the const qualifier from member functions that return non-const pointers or references, or otherwise fail to meet the logical constness test. From the tone of the initial e-mail it sounded like there was some desire to get rid of const declarations across the board. I would be opposed to this change. I think I may have gone a little too far in venting my own confusion about the role of const member functions in C++ -- to stimulate discussion, I made a strawperson proposal not to use const member functions at all, but I don't think anyone actually supports that proposal, even though there is some support for removing incorrect or unhelpful uses of const member functions. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Thu, Jun 2, 2011 at 1:02 PM, Geoffrey Garen gga...@apple.com wrote: Perhaps we could at least encourage const-correctness for newly-written classes? By this I mean both adherence to the logical-constness rules you stated earlier as well as not adding non-const accessors and members unless needed -- otherwise it's easy to just err on the side of never using const anywhere. I'm still not convinced that it's meaningful to talk about a const-correct class. The const that you put on a member function only has meaning when somebody uses a const pointer or reference to your class. More importantly, any design issues raised or bugs caught by const only get tested when somebody uses a const pointer or reference to your class. So, the key to const-correctness is deciding when to use const pointers and const references -- once you're using a const pointer or reference, the compiler will tell you which member functions need to be const. Hmm. Relying on compilers to check constness is inherently scary because they check physical constness, which is neither a superset nor subset of logical constness, which is what we want to be checking. At least in my head, my design process is generally the class should expose as few APIs as possible, and they should be const wherever possible (without violating logical constness). Obviously in some sense classes aren't built in a vacuum and their external users determine the APIs they expose, but I don't like the constness is determined purely by external need route because it leads to problems like the first user of this class wanted to mutate everything, so I made no members const, and now I can't trivially add a non-mutating const usage because it requires plumbing constness through everywhere. Maybe what I'm trying to say is that it's generally feasible to design with make usage patterns const where possible in mind, but it's harder to retrofit. Or maybe I'm saying it's easier to make things non-const later than to make them const. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On May 31, 2011, at 10:00 PM, Brent Fulgham wrote:
On May 31, 2011, at 8:44 PM, Maciej Stachowiak wrote:
For example, the compiler does not tell you that the following
implementation of Node::previousSibling() (currently in our code!) is
totally wrong from the logical const perspective:
Node* previousSibling() const { return m_previous; }
The correct way to do const-correctness here would require more verbosity:
const Node* previousSibling() const { return m_previous; }
Node* previousSibling() { return m_previous; }
And the first variant would be dead code if we don't ever actually use const
node pointers.
Given your views on const-correctness discipline, what do you think is the
right way to handle this situation? Note that this pattern comes up for many
core DOM methods and many render tree methods.
One possible (though ugly) way of allowing the compiler to do some of this
work for you is to declare the m_previous member as a const pointer, and then
manipulate it only in specific routines using the const_cast operator to
allow it to mutate. But this is probably a case of the cure being worse than
the disease.
This cure would most definitely be worse than the disease. All JavaScript
access to the DOM gets (effectively) non-const node references, so we'd need to
cast away const everywhere.
If we had logic that iterated over the node siblings in read-only fashion, we
would ideally do so through a const iterator. In addition to documenting
that we don't intend to mutate the object, it would provide potentially
useful information that compilers could use to make aliasing decisions and so
forth. Perhaps we never iterate over DOM nodes without intending to mutate
them; if so, I would agree that there is no benefit to maintaining the const
variation.
It's my understanding that compilers make essentially no use of a method being
labeled const to enable optimizations. Given const_cast and mutable, just
knowing the signature is const is not a strong enough guarantee to decide
anything at the call site. And if the compilercan see the body of the function,
and see that it actually doesn't mutate the object, it know all it needs to
know and are not helped by the const declaration. Marking methods as const to
help the compiler is, as far as I know, simply cargo cult engineering.
However I do not think (as the Node example might imply) that the fact that
the compiler cannot identify ALL categories of const error means that we are
better off washing our hands of const correctness.
Let's set aside the compiler's error checking for the moment. What do you
believe is the proper const-correct way to write previousSibling() and related
methods? A priori, I think the correct way is this:
Node* previousSibling() { return m_previous; }
I could also be convinced that the following is technically more correct,
though in a way that is completely useless for our code base at present:
const Node* previousSibling() const { return m_previous; }
Node* previousSibling() { return m_previous; }
The following would be valid but would require us to cast away const all over
the place and would therefore in my opinion be a net negative:
const Node* previousSibling() const { return m_previous; }
And what's in our code now violates the intent of const, and so to me at least
is clearly a bug:
Node* previousSibling() const { return m_previous; }
What do you think is the right way to do it? One of these? Something else?
In fact, due to the viral nature of const-ness changes, leaving them in (and
continuing to maintain them) is a good long term approach since the first
time you want to work with a const Node object you will have to resurrect
const variations of methods across the object hierarchy.
Well one big problem right now (just from scanning the core DOM classes) is
that we have a lot of clearly broken use of const. We could (a) leave it as-is,
(b) remove the incorrect use of const, or (c) deploy proper const-correctness.
it seems like you are against (b), but I cannot tell if you advocate (a) or (c).
Regards,
Maciej
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Tue, May 31, 2011 at 11:31 PM, Maciej Stachowiak m...@apple.com wrote:
The following would be valid but would require us to cast away const all
over the place and would therefore in my opinion be a net negative:
const Node* previousSibling() const { return m_previous; }
And what's in our code now violates the intent of const, and so to me at
least is clearly a bug:
Node* previousSibling() const { return m_previous; }
I agree that these are both bad.
Well one big problem right now (just from scanning the core DOM classes) is
that we have a lot of clearly broken use of const. We could (a) leave it
as-is, (b) remove the incorrect use of const, or (c) deploy proper
const-correctness. it seems like you are against (b), but I cannot tell if
you advocate (a) or (c).
Misusing const is IMO worse than not using const. I would be in favor of
removing cases like the last one you cite above, as well as working to
remove const_casts, at least where they indicate problems.
It would be nice to make things more properly const-correct, and make
callers use const accessors when possible (e.g. use the const form of tree
iteration when walking a tree merely to read data and not write it), so I
don't think we should forbid good use of const, but it's also time-consuming
to retrofit.
Perhaps we could at least encourage const-correctness for newly-written
classes? By this I mean both adherence to the logical-constness rules you
stated earlier as well as not adding non-const accessors and members unless
needed -- otherwise it's easy to just err on the side of never using const
anywhere.
I also think we should not discourage people from using const on
variables. I've gotten review comments in the past that have asked me to
remove const qualifiers on locals, as well as sometimes on arguments and
members, and I think this is a mistake. We aren't const-correct elsewhere
in WebCore is not a good reason to forbid const.
PK
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Tue, May 31, 2011 at 11:31 PM, Maciej Stachowiak m...@apple.com wrote:
[...] What do you believe is the proper const-correct way to write
previousSibling() and related methods?
A priori, I think the correct way is this:
Node* previousSibling() { return m_previous; }
I could also be convinced that the following is technically more correct,
though in a way that is completely
useless for our code base at present:
const Node* previousSibling() const { return m_previous; }
Node* previousSibling() { return m_previous; }
What do you think is the right way to do it? One of these? Something else?
[...]
Well one big problem right now (just from scanning the core DOM classes) is
that we have a lot of clearly
broken use of const. We could (a) leave it as-is, (b) remove the incorrect
use of const, or (c) deploy proper
const-correctness. it seems like you are against (b), but I cannot tell if
you advocate (a) or (c).
I would *prefer* to deploy proper const correct accessors, so (c).
However, in the interests of pragmatism I think that it would be
reasonable to at least remove the improper uses of const (b).
From the tone of the initial e-mail it sounded like there was some
desire to get rid of const declarations across the board. I would be
opposed to this change. However, I concede that improper use of const
is worse than no const declaration, and would support your (b) case
(though I would prefer (c)!).
-Brent
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Tue, May 31, 2011 at 12:08 PM, Geoffrey Garen gga...@apple.com wrote: I agree that const should be used for logical constness. The rule should not be merely doesn't alter any data members of this object but rather does not alter observable state of this object or vend any type of pointer or reference by which observable state of this object could be altered. Precisely! I like this explanation too. I'm trying to think of a simple way to explain / test whether something falls into the category of logical constness, since it can be ambiguous. It occurred to me that a simple, though imperfect, test is just, Is this function called by an owner of a const pointer / reference? After all, a const member function is meaningless if nobody points to the class through a const pointer / reference. For classes like DOM and render tree nodes, which have no meaningful const-pointer-owning clients, the answer is always no. For other classes, the answer is yes, but only if someone has found a meaningful use for a const pointer or reference to the class. So, perhaps the real style question we should answer is when to use const pointers / references, since the answer to when to use const member functions will follow from it. What are some good examples of existing code that meaningfully uses a const pointer or reference? (Something other than, say, the obligatory const in a copy constructor.) I personally find that in a number of render tree functions declared to take a const RenderObject* that the const-ness is a useful hint that the function will not be manipulating the object in unexpected ways. Examples: http://codesearch.google.com/codesearch?hl=envert=chromiumlr=q=%22const+RenderObject%22+file%3AWebCoresbtn=Search (click Next at the bottom to see more results). My understanding is that these functions could not take a const RenderObject* parameter if RenderObject did not supply a set of const simple accessors and utility functions. We could convert RenderObject over to have no const member functions and convert all of these utility functions to take RenderObject* parameters instead, but I think that would lose some of the intent of the code. I'm curious if there was a specific patch or piece of code that lead you to send this email out - perhaps we make a better decision about whether to change our approach with more concrete examples of problems the current situation has caused or is causing. - James Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On May 30, 2011, at 4:19 PM, Geoffrey Garen wrote: Updated: Const member functions: Do use const member functions in classes that are independent data holders, to help distinguish between references that can modify the data and references that can't. Do not use const member functions in classes that participate in object graphs, since the distinction is weak. Do not use const member functions for DOM or render tree nodes. While I agree as to the DOM or render tree, I'm not sure this statement is right in general. A linked list node or tree node could useful have const methods, which give only const pointers/references to other nodes. If there is a reason const references to DOM nodes or renew objects are not useful, it is not due to the object graph participation itself, in my opinion. Regards, Maciej Geoff On May 30, 2011, at 4:08 PM, Ryosuke Niwa wrote: On Mon, May 30, 2011 at 4:02 PM, Geoffrey Garen gga...@apple.com wrote: Do not use const member functions in complex classes that do a lot more than hold one piece of data I'm not sure if the complexity of a class or holding piece of data are useful criteria. Looking at DOM or render tree, it seems that we don't want to use const when an object constitutes (i.e. object's data members are essential in creating) a larger data structure such as a tree or a list. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
Even in a class that is used in a tree, I still think simple member variable accessor methods (that do not return tree neighbors) should be const. OK. Why? Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On May 31, 2011, at 10:47 AM, Geoffrey Garen wrote: Even in a class that is used in a tree, I still think simple member variable accessor methods (that do not return tree neighbors) should be const. OK. Why? Because it indicates to me and the compiler, that the method doesn't have side effects. Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Mon, May 30, 2011 at 11:32 PM, Maciej Stachowiak m...@apple.com wrote: A linked list node or tree node could useful have const methods, which give only const pointers/references to other nodes. If there is a reason const references to DOM nodes or renew objects are not useful, it is not due to the object graph participation itself, in my opinion. Indeed. The rule of thumb I use is that a const member function should only return const-ref or pointer-to-const objects (whether as return values or outparams). This helps ensure that the method is logically const, not just physically const, by preventing callers from using const methods to gain handles they can use to modify supposedly-const objects. It so happens that objects in trees/graphs tend to have functions that return pointers to other objects much more frequently than do independent data holders. Thus Geoff's rule ends up approximating my rule, but is not equivalent. As to use of const in general, I would prefer to see more of it rather than less, _assuming it is used only for logical constness_. See Scott Meyers' Effective C++, item 3, for rationale. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
Even in a class that is used in a tree, I still think simple member variable accessor methods (that do not return tree neighbors) should be const. OK. Why? Because it indicates to me and the compiler, that the method doesn't have side effects. A const member function can have side effects. It can modify any global state outside the object. It can also modify sub-objects inside the object, and return non-const references to sub-objects and related objects that might be used to produce side-effects at any time. It's exactly statements like this that make me think that const member functions are a bad idea -- people think they provide a guarantee, but they don't. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On May 31, 2011, at 10:54 AM, Peter Kasting wrote: On Mon, May 30, 2011 at 11:32 PM, Maciej Stachowiak m...@apple.com wrote: A linked list node or tree node could useful have const methods, which give only const pointers/references to other nodes. If there is a reason const references to DOM nodes or renew objects are not useful, it is not due to the object graph participation itself, in my opinion. Indeed. The rule of thumb I use is that a const member function should only return const-ref or pointer-to-const objects (whether as return values or outparams). This helps ensure that the method is logically const, not just physically const, by preventing callers from using const methods to gain handles they can use to modify supposedly-const objects. It so happens that objects in trees/graphs tend to have functions that return pointers to other objects much more frequently than do independent data holders. Thus Geoff's rule ends up approximating my rule, but is not equivalent. As to use of const in general, I would prefer to see more of it rather than less, _assuming it is used only for logical constness_. See Scott Meyers' Effective C++, item 3, for rationale. I agree that const should be used for logical constness. The rule should not be merely doesn't alter any data members of this object but rather does not alter observable state of this object or vend any type of pointer or reference by which observable state of this object could be altered. This explains both why mutable data members are ok (they are meant for cache/memoization purposes, and do not alter observable state) and why const methods returning non-const pointers to other objects in a mutually referencing graph are not. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Tue, May 31, 2011 at 11:00 AM, Maciej Stachowiak m...@apple.com wrote: I agree that const should be used for logical constness. The rule should not be merely doesn't alter any data members of this object but rather does not alter observable state of this object or vend any type of pointer or reference by which observable state of this object could be altered. Precisely! Because this is subtle, and can't be completely checked by a compiler, people can often get it wrong. Used correctly, though, const is a powerful tool for enforcing correct API usage, allowing optimizations, and informing readers of a class' contract and functionality. Reviewers should understand and consider these sorts of details. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
I agree that const should be used for logical constness. The rule should not be merely doesn't alter any data members of this object but rather does not alter observable state of this object or vend any type of pointer or reference by which observable state of this object could be altered. Precisely! I like this explanation too. I'm trying to think of a simple way to explain / test whether something falls into the category of logical constness, since it can be ambiguous. It occurred to me that a simple, though imperfect, test is just, Is this function called by an owner of a const pointer / reference? After all, a const member function is meaningless if nobody points to the class through a const pointer / reference. For classes like DOM and render tree nodes, which have no meaningful const-pointer-owning clients, the answer is always no. For other classes, the answer is yes, but only if someone has found a meaningful use for a const pointer or reference to the class. So, perhaps the real style question we should answer is when to use const pointers / references, since the answer to when to use const member functions will follow from it. What are some good examples of existing code that meaningfully uses a const pointer or reference? (Something other than, say, the obligatory const in a copy constructor.) Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On May 31, 2011, at 12:08 PM, Geoffrey Garen wrote: I agree that const should be used for logical constness. The rule should not be merely doesn't alter any data members of this object but rather does not alter observable state of this object or vend any type of pointer or reference by which observable state of this object could be altered. Precisely! I like this explanation too. I'm trying to think of a simple way to explain / test whether something falls into the category of logical constness, since it can be ambiguous. It occurred to me that a simple, though imperfect, test is just, Is this function called by an owner of a const pointer / reference? After all, a const member function is meaningless if nobody points to the class through a const pointer / reference. For classes like DOM and render tree nodes, which have no meaningful const-pointer-owning clients, the answer is always no. For other classes, the answer is yes, but only if someone has found a meaningful use for a const pointer or reference to the class. So, perhaps the real style question we should answer is when to use const pointers / references, since the answer to when to use const member functions will follow from it. What are some good examples of existing code that meaningfully uses a const pointer or reference? (Something other than, say, the obligatory const in a copy constructor.) I agree that one useful distinction is whether a particular kind of object is every manipulated via const pointers or references. If we never use const references/pointers to a particular kind of object, then it is probably not useful to maintain const-correctness discipline for that class. If we wanted to make the DOM or render tree thoroughly const-correct, the key question would be what client code would access via const pointers or references. For container classes, it is almost certain that some client will wish to vend or consume const references, even if the very first client doesn't. There's probably other classes where it's less clear-cut. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
Hi, On Tue, May 31, 2011 at 4:37 PM, Maciej Stachowiak m...@apple.com wrote: I agree that one useful distinction is whether a particular kind of object is every manipulated via const pointers or references. If we never use const references/pointers to a particular kind of object, then it is probably not useful to maintain const-correctness discipline for that class. I don't agree with this. I see no harm in maintaining const correctness on objects, even if they are not currently manipulated through const references/pointers. They provide clear, self-documenting evidence that the object design intends that certain methods do not visibly alter the object. I fail to see how abandoning const correctness can do anything but cause us to start missing bugs that we would otherwise have found via compiler errors. -Brent ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On May 31, 2011, at 5:55 PM, Brent Fulgham wrote:
Hi,
On Tue, May 31, 2011 at 4:37 PM, Maciej Stachowiak m...@apple.com wrote:
I agree that one useful distinction is whether a particular kind of object
is every manipulated via const pointers or references. If we never use const
references/pointers to a particular kind of object, then it is probably not
useful to maintain const-correctness discipline for that class.
I don't agree with this. I see no harm in maintaining const
correctness on objects, even if they are not currently manipulated
through const references/pointers. They provide clear,
self-documenting evidence that the object design intends that certain
methods do not visibly alter the object.
I fail to see how abandoning const correctness can do anything but
cause us to start missing bugs that we would otherwise have found via
compiler errors.
It's hard for me to evaluate this in the abstract. In general I like the
compiler telling me as much as possible. But there's both false positive and
false negative problems with const-correctness, and a maintenance burden
besides just fixing the compiler errors.
For example, the compiler does not tell you that the following implementation
of Node::previousSibling() (currently in our code!) is totally wrong from the
logical const perspective:
Node* previousSibling() const { return m_previous; }
The correct way to do const-correctness here would require more verbosity:
const Node* previousSibling() const { return m_previous; }
Node* previousSibling() { return m_previous; }
And the first variant would be dead code if we don't ever actually use const
node pointers.
Given your views on const-correctness discipline, what do you think is the
right way to handle this situation? Note that this pattern comes up for many
core DOM methods and many render tree methods.
Regards,
Maciej
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On May 31, 2011, at 8:44 PM, Maciej Stachowiak wrote:
For example, the compiler does not tell you that the following implementation
of Node::previousSibling() (currently in our code!) is totally wrong from the
logical const perspective:
Node* previousSibling() const { return m_previous; }
The correct way to do const-correctness here would require more verbosity:
const Node* previousSibling() const { return m_previous; }
Node* previousSibling() { return m_previous; }
And the first variant would be dead code if we don't ever actually use const
node pointers.
Given your views on const-correctness discipline, what do you think is the
right way to handle this situation? Note that this pattern comes up for many
core DOM methods and many render tree methods.
One possible (though ugly) way of allowing the compiler to do some of this work
for you is to declare the m_previous member as a const pointer, and then
manipulate it only in specific routines using the const_cast operator to
allow it to mutate. But this is probably a case of the cure being worse than
the disease.
If we had logic that iterated over the node siblings in read-only fashion, we
would ideally do so through a const iterator. In addition to documenting that
we don't intend to mutate the object, it would provide potentially useful
information that compilers could use to make aliasing decisions and so forth.
Perhaps we never iterate over DOM nodes without intending to mutate them; if
so, I would agree that there is no benefit to maintaining the const variation.
However I do not think (as the Node example might imply) that the fact that the
compiler cannot identify ALL categories of const error means that we are better
off washing our hands of const correctness.
In fact, due to the viral nature of const-ness changes, leaving them in (and
continuing to maintain them) is a good long term approach since the first time
you want to work with a const Node object you will have to resurrect const
variations of methods across the object hierarchy.
-Brent
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
OK, how about this style guideline: Const member functions: Do use const member functions in classes that are simple data holders, to help distinguish between references that can modify the data and references that can't. Do not use const member functions in complex classes that do a lot more than hold one piece of data, since the distinction is weak. Do not use const member functions for DOM or render tree nodes. Geoff On May 29, 2011, at 1:20 PM, Darin Adler wrote: On May 28, 2011, at 5:15 PM, Geoffrey Garen wrote: This came up in https://bugs.webkit.org/show_bug.cgi?id=59604. My personal preference is against const member functions because they force many error-prone code changes: introducing a const forces the transitive closure of all potential callees to change their signatures recursively, and if you get this wrong in the case of a virtual function, you introduce a very subtle set of bugs. I think we do currently overuse const. There are many objects that are not really data holders, and it does not seem all that helpful to distinguish a const member function on such an object. I don't see much upside to const member functions because they're just an unenforced convention: sub-object and mutable data members are still non-const inside a so-called const function, and you can always const_cast away so-called const-ness, as WebKit currently does in 483 places. I don’t fully agree. Although const is not an airtight mechanism, it can be helpful to make clear which functions are intended to have side effects and which are not. It’s helped me notice programming mistakes in the past. I don’t think the presence of mutable invalidates the helpfulness of const. I also don’t think the total number of calls to const_cast in WebKit is a good metric of how well const is working as a design pattern. Many of those calls are required due to working with libraries outside of WebKit such as CoreFoundation. I think we should stop using const member functions on classes where the distinction is weak and it’s not paying off. The classes that immediately come to mind are the DOM elements and render tree nodes. Having a const pointer to one node in a tree is not all that meaningful since the const-ness is immediately lost if you move to a neighbor. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Mon, May 30, 2011 at 4:02 PM, Geoffrey Garen gga...@apple.com wrote: Do not use const member functions in *complex classes that do a lot more than hold one piece of data* I'm not sure if the complexity of a class or holding piece of data are useful criteria. Looking at DOM or render tree, it seems that we don't want to use const when *an object constitutes (i.e. object's data members are essential in creating) a larger data structure such as a tree or a list*. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
Updated: Const member functions: Do use const member functions in classes that are independent data holders, to help distinguish between references that can modify the data and references that can't. Do not use const member functions in classes that participate in object graphs, since the distinction is weak. Do not use const member functions for DOM or render tree nodes. Geoff On May 30, 2011, at 4:08 PM, Ryosuke Niwa wrote: On Mon, May 30, 2011 at 4:02 PM, Geoffrey Garen gga...@apple.com wrote: Do not use const member functions in complex classes that do a lot more than hold one piece of data I'm not sure if the complexity of a class or holding piece of data are useful criteria. Looking at DOM or render tree, it seems that we don't want to use const when an object constitutes (i.e. object's data members are essential in creating) a larger data structure such as a tree or a list. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On May 30, 2011, at 4:19 PM, Geoffrey Garen wrote: Updated: Const member functions: Do use const member functions in classes that are independent data holders, to help distinguish between references that can modify the data and references that can't. Do not use const member functions in classes that participate in object graphs, since the distinction is weak. Do not use const member functions for DOM or render tree nodes. Even in a class that is used in a tree, I still think simple member variable accessor methods (that do not return tree neighbors) should be const. Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On May 28, 2011, at 5:15 PM, Geoffrey Garen wrote: This came up in https://bugs.webkit.org/show_bug.cgi?id=59604. My personal preference is against const member functions because they force many error-prone code changes: introducing a const forces the transitive closure of all potential callees to change their signatures recursively, and if you get this wrong in the case of a virtual function, you introduce a very subtle set of bugs. I think we do currently overuse const. There are many objects that are not really data holders, and it does not seem all that helpful to distinguish a const member function on such an object. I don't see much upside to const member functions because they're just an unenforced convention: sub-object and mutable data members are still non-const inside a so-called const function, and you can always const_cast away so-called const-ness, as WebKit currently does in 483 places. I don’t fully agree. Although const is not an airtight mechanism, it can be helpful to make clear which functions are intended to have side effects and which are not. It’s helped me notice programming mistakes in the past. I don’t think the presence of mutable invalidates the helpfulness of const. I also don’t think the total number of calls to const_cast in WebKit is a good metric of how well const is working as a design pattern. Many of those calls are required due to working with libraries outside of WebKit such as CoreFoundation. I think we should stop using const member functions on classes where the distinction is weak and it’s not paying off. The classes that immediately come to mind are the DOM elements and render tree nodes. Having a const pointer to one node in a tree is not all that meaningful since the const-ness is immediately lost if you move to a neighbor. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Do we have a style preference about const member functions?
This came up in https://bugs.webkit.org/show_bug.cgi?id=59604. My personal preference is against const member functions because they force many error-prone code changes: introducing a const forces the transitive closure of all potential callees to change their signatures recursively, and if you get this wrong in the case of a virtual function, you introduce a very subtle set of bugs. I don't see much upside to const member functions because they're just an unenforced convention: sub-object and mutable data members are still non-const inside a so-called const function, and you can always const_cast away so-called const-ness, as WebKit currently does in 483 places. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
