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

[Peter Kasting]

On Fri, Jun 3, 2011 at 5:59 PM, Darin Adler <darin at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20110607/3509108c/attachment.html>