On Fri, Jun 3, 2011 at 5:59 PM, Darin Adler <[email protected]> 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 [email protected] http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
