[webkit-dev] Pointers and self-documenting code

[Ryosuke Niwa]

On Fri, Jul 6, 2012 at 1:35 PM, Joe Mason <jmason at rim.com> wrote:

> As has been noted in a recent thread, WebKit strives to make the code
> clear and understandable rather than have embedded comments that can become
> obsolete.  One common practice that I find quite opaque is for classes to
> have functions returning pointers which can never return 0, especially when
> such functions are often chained.
>
> For example, is this call safe?
>
> int fontSize =
> node->document()->frame()->page()->settings()->minimumFontSize();
>
> (Yes, I could call document()->settings() directly, but I want to
> illustrate all the different styles of writing accessors.)
>
> No it is not:
>
> Node::document() returns m_document, with an assert documenting that it
> never returns 0
> Document::frame() has a comment stating that it CAN return 0
> Frame::page() CAN return 0 - I think.  It just returns "m_page" with no
> comments - is this the same situation as Node::document(), so m_page is
> never 0, but nobody bothered to assert?  I deduce that m_page can be 0
> because the detachFromPage() function which sets m_page to 0 is defined
> next in the file and I happened to look down and notice it.
> Page::settings() cannot return 0, since it returns m_settings.get() and
> m_settings is an OwnPtr, so I know its lifetime matches that of the Page.
>
> So the correct way to write this is:
>
> int fontSize = 0;
> Frame* frame = node->document()->frame();
> if (frame) {
>     Page* page = frame->page();
>     if (page)
>         fontSize = page->settings()->minimumFontSize();
> }
>
> But the only way to know this is to read the code for each accessor!  And
> forget about memorizing which is which: Document::settings() can return 0,
> but Page::settings() can't, so whether "settings()" needs to be checked or
> not depends on the context.
>
> Is there a reason we don't return a reference from functions that are
> guaranteed to be non-zero?  Then the above would become:
>
> int fontSize =
> node->document().frame()->page()->settings().minimumFontSize();
>
> And the error is easy to spot: all the dereferences without checking for 0
> are dangerous.  And the compiler will enforce this.
>

That sounds like a good idea. I like it. I'd like to hear opinions of more
"senior" contributors though.

- Ryosuke
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120706/693cdfd8/attachment.html>