[webkit-dev] Do we have a style preference about const member functions?

[Maciej Stachowiak]

On Jun 8, 2011, at 11:48 AM, Peter Kasting wrote:

> On Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak <mjs at 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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20110609/a99f3953/attachment.html>