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

[Peter Kasting]

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.

(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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20110608/43f097c5/attachment.html>