[webkit-dev] On returning mutable pointers from const methods

[Ojan Vafai]

On Thu, Oct 25, 2012 at 5:04 AM, Stephen Chenney <schenney at chromium.org>wrote:

> I second the principle. I ran into this kind of code pattern recently in
> fonts and was unnerved by the thought of a mutable pointer emerging from a
> const method.
>
> I am not at all certain that we can simply make the change without some
> heavy refactoring. On the one hand, there are some long const-ness chains
> in some places that removing const from a method will break. On the other
> hand, making the returned pointer const in some places will force
> additional const-ness or require changes to code that uses the pointer.
>
> Do we try to apply the new style to new code only?
>

This is typically how we handle all style rules. They apply to new code and
to code you are modifying, but there's no expectation to clean up old code
just for the sake of meeting the style rule. Although, I think in this
case, if someone felt moved, it might be acceptable since this isn't just a
readability issue and will likely uncover real bugs.


> Stephen.
>
> On Thu, Oct 25, 2012 at 6:48 AM, Andreas Kling <kling at webkit.org> wrote:
>
>> Yo WebKittens!
>>
>> After some mild morning discussion in #webkit, I'm wondering if we should
>> amend our style guide to disallow returning mutable pointers (Foo*) from
>> const methods, like so:
>>
>> - Foo* foo() const;
>>
>> While this is useful when you want to be able to take a strong reference
>> to the returned object, i.e by assigning it to a RefPtr<Foo>, it's
>> counter-intuitive as it encourages "read-only" methods to hand out
>> "read/write" references to internal data.
>>
>> I've been using const-ness to prevent certain classes of bugs at compile
>> time for the immutable/mutable implementations of WebCore's
>> ElementAttributeData and StylePropertySet. It's especially important that
>> the immutable versions of these objects don't have their internal data
>> modified by unwitting external clients, since they may be shared between
>> multiple owners. While using assertions helps us catch incorrect use of
>> these objects, it's obviously optimal to catch the bugs already at compile
>> time.
>>
>> I just now discovered that you can grab at a mutable CSSValue* pointer in
>> an immutable "const StylePropertySet*" by going through
>> propertyAt(index).value(), since we have "CSSValue* CSSProperty::value()
>> const". This is bad, because it means that we have a bunch of call sites
>> operating on mutable pointers into what's supposed to be an immutable
>> object!
>>
>> While I haven't identified any real bugs caused by this (yet), it would
>> be easy to introduce one by mistake.
>>
>> So, I propose that we allow only these two signature formats for raw
>> pointers:
>>
>> - const Foo* foo() const;
>> - Foo* foo();
>>
>> Moreover, for methods that return references to objects where call sites
>> are expected to take a strong reference, we should really be using
>> PassRefPtr<Foo> as the return type.
>>
>> Thoughts? Objections? Am I missing something?
>>
>> -Kling
>>
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org
>> http://lists.webkit.org/mailman/listinfo/webkit-dev
>>
>>
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20121025/d706d17b/attachment.html>