Re: [webkit-dev] Do we have a style preference about const member functions?
On Thu, Jun 9, 2011 at 3:51 PM, Maciej Stachowiak 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 9, 2011, at 11:13 AM, Peter Kasting wrote: > On Thu, Jun 9, 2011 at 2:49 AM, Maciej Stachowiak 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. > 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. The compiler won't protect you from that mistake, just as it won't protect you from casting away const on 'this' and then calling a non-const method with a side effect. Maybe this is paranoid, but not much more paranoid than worrying about accessors suddenly sprouting observable side effects. > Casting away const on |this|, OTOH, allows you to mutate objects even when > you never had permission to begin with. Much different. const is a hint, not "permission", since the caller can cast away const directly. Think of const as a practical tool to help document behaviors, not as an enforced security model. > In any case, my intent is to proceed as Darin and I discussed. Wounds good! 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 Thu, Jun 9, 2011 at 2:49 AM, Maciej Stachowiak 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 Jun 9, 2011, at 2:49 AM, Maciej Stachowiak wrote: > > On Jun 8, 2011, at 11:48 AM, Peter Kasting wrote: > >> On Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak 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. Now having read the rest of the thread, I think you should start by posting what you and Darin agreed to. - 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 8, 2011, at 11:48 AM, Peter Kasting wrote: > On Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak 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 Wed, Jun 8, 2011 at 11:59 AM, Darin Adler 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 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:51 AM, Darin Adler wrote: > On Jun 8, 2011, at 11:48 AM, Peter Kasting wrote: > > On Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak > 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:48 AM, Peter Kasting wrote: > On Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak 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 Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak 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 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?
I think the “make some things non-const” part of this would be the first part to put up for review and land. -- 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:59 PM, Darin Adler 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 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 Fri, Jun 3, 2011 at 5:27 PM, Darin Adler 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 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 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 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 Thu, Jun 2, 2011 at 1:02 PM, Geoffrey Garen 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?
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=en&vert=chromium&lr=&q=%22const+RenderObject%22+file%3AWebCore&sbtn=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/webki
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 : > 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 : > 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?
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 : 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?
On Thu, Jun 2, 2011 at 12:38 AM, Maciej Stachowiak wrote: > > 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. > 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 because we use a raw pointer to keep track of the current node when walking up the tree. - 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 Tue, May 31, 2011 at 12:08 PM, Geoffrey Garen 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?
On Jun 1, 2011, at 8:11 PM, James Robinson wrote: > On Tue, 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 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=en&vert=chromium&lr=&q=%22const+RenderObject%22+file%3AWebCore&sbtn=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 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=en&vert=chromium&lr=&q=%22const+RenderObject%22+file%3AWebCore&sbtn=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 Tue, May 31, 2011 at 11:31 PM, Maciej Stachowiak 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 11:31 PM, Maciej Stachowiak 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 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 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?
On May 31, 2011, at 5:55 PM, Brent Fulgham wrote: > Hi, > > On Tue, May 31, 2011 at 4:37 PM, Maciej Stachowiak 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?
Hi, On Tue, May 31, 2011 at 4:37 PM, Maciej Stachowiak 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 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?
> 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 Tue, May 31, 2011 at 11:00 AM, Maciej Stachowiak 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?
On May 31, 2011, at 10:54 AM, Peter Kasting wrote: > On Mon, May 30, 2011 at 11:32 PM, Maciej Stachowiak 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?
>>> 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 Mon, May 30, 2011 at 11:32 PM, Maciej Stachowiak 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?
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?
> 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 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 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?
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?
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 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 Mon, May 30, 2011 at 4:02 PM, Geoffrey Garen 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?
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 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