[webkit-dev] Pointers and self-documenting code

[Filip Pizlo]

On Jul 6, 2012, at 1:35 PM, Joe Mason 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();
> }

It's not at all clear that this is correct.

Is it correct for the font size to be zero if the frame doesn't exist?  Seems dubious.

Is it correct for the font size to be zero if the page doesn't exist?  Seems dubious.

Adding null checking only makes sense if you have a good story for what the code ought to do if the pointer in question is null.

> 
> 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.

This is a questionable policy.  Often object properties have transient nullness.  They may be null at time T1 and non-null at time T2, and the caller may know that the property is non-null from context (the value of some other field, the fact that it's performed some action that forces the field to become non-null, etc).

Thus statically enforcing the non-nullness of fields is likely to just make the code more complicated.  And it doesn't buy anything.

-F


> 
> I find the common use of things like "document()->frame()" without checking the return value of document() is a big impediment to understanding the code, because I'm never sure if I can take it as gospel or if someone has been sloppy and forgotten to check return values.  And in turn it leads to sloppiness, since it's easy to forget which accessors do require null checks and write an invalid chain such as the one above.
> 
> Joe
> 
> ---------------------------------------------------------------------
> This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev