On Jun 8, 2011, at 11:48 AM, Peter Kasting wrote: > On Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak <[email protected]> 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 [email protected] http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
