[webkit-dev] …Inlines.h vs …InlineMethods.h

[Filip Pizlo]

On Nov 5, 2012, at 9:27 PM, Geoffrey Garen <ggaren at apple.com> wrote:

>>> (5) Adopt the convention that any function that is not as trivial as "int x() { return m_x; }" moves out of the class declaration.
>> 
>> How about we simplify this slightly to:
>> 
>> (5) Adopt the convention that any function that is nontrivial should be moved out of the class declaration.
>> 
>> We can give an example as to what might constitute trivial if we wish (e.g. is a one liner), but I think leaving a little wiggle room to allow developers to apply common sense would be a good thing.  While moving all complex functions out of class definitions sounds good, for some small classes being able to leave some very simple functions in the class declaration might not hurt, and might make the code easier to work with.  E.g.:
>> 
>> int y()
>> {
>> 	ASSERT(m_y != BAD_VALUE);
>> 	return m_y;
>> }
> 
> If possible, I'd like to establish clarity on what "trivial" or "nontrivial" means. This can save us debates in future patch reviews about what's "trivial enough".
> 
> To me, "trivial" means short. My straw-man proposal is "one-line functions only".
> 
> Failing that, I would at least like to pick some number. Maybe 6 lines, since that's just enough for a branch with an early return.
> 
> Thought complexity notwithstanding, non-trivial functions mainly get in the way of reading an interface because they take up space. "int x() { return m_x; }" is fine by me because it doesn't add any lines of code over "int x();". Notably, the next shortest function possible in WebKit style after one line is five lines. That means that I see 5X less of the class declaration in one screenful of code, and I have to do 5X more scrolling before I can see the data members in a class. To me, that's a significant blow to readability for a slight improvement in write-ability. Given that reading is more common than writing, I'm inclined to argue that >1 line functions are not worth it.
> 
> In general, I think brevity in class declarations is particularly important. I often find myself needing to read all of the public interfaces of a class, or look at a declaration in relation to a prior "public" or "private" keyword, or scroll past the interface declarations to get to the data members. (In contrast, I rarely need to read all of the function implementations of a class at once, or scroll to a specific lexical location among a set of function implementations.) Within some limits, I'm willing to write code more slowly so I can read declarations more quickly.

I prefer wiggle room for decisions regarding where to put method bodies.  I agree that we should use *Inlines.h instead of hijacking other class's headers.  But beyond that, I would prefer to go with Gavin's suggestion rather than imposing a rigid rule.

To me the decision of where to put a method body comes down to weighing two use cases:

A) Reading the interface that a class provides.

B) Making changes to the class.

Inline methods being truly inline in the class body aids (B) while impeding (A).  Non-tiny methods being out-of-line aids (A) while impeding (B).

For most of the classes with which I am familiar, (B) appears to be more common than (A) by virtue of those classes having very few consumers.  I would guess that the typical class in JavaScriptCore is only directly used from a handful places.  So, it's uncommon that you're just going to be wondering about the class's interface.  It's much more likely that if you're thinking about that class, you're goal is to change it.  At that point, having more of the class's guts collocated in one place is a good thing.  (Of course for methods that ought to be out-of-line in a .cpp file there's nothing we can do - but at least we can simplify life for inline methods.)

I suspect that there are also classes for which (A) is an unlikely use case just because it has low utility - say, because it's a gnarly enough class that just knowing the method names reveals too little information.

MarkedBlock.h is a good example of a class that has good encapsulation, but that has very few consumers by virtue of it being a class that is mostly internal to one of the two spaces managed by our GC.  Hence, the most common "use case" for touching MarkedBlock.h is not to read how it interacts with other classes (since there are not many classes that it interacts with) but to change MarkedBlock itself.  Whenever I have to touch MarkedBlock, I find myself annoyed by methods appearing in two places.  Adding methods and changing their signatures is tedious.  And understanding the methods is also tedious - for example I rarely want to know whether MarkedBlock has an isLive() method, but I often want to know how isLive() works.

A counterexample to this would be something like Vector, where (A) is orders of magnitude more likely than (B).  Also, I rarely want to know how Vector works.  I'm happy to assume that it just does the good things.  So of course for Vector, having non-tiny methods placed out-of-line is better.  But the fact that we do it for MarkedBlock buys me nothing and generally makes me sad.

I think there are other examples, in addition to MarkedBlock.  It's not a lone outlier.  Most of the DFG classes behave this way, by virtue of having relationships with only a few other DFG classes.  So, having two lines of code for the method signature of each method in those classes, where just one method signature would have sufficed, would just be tedious.

As a sidenote, I consider the spacial separation of interface from definition to just be an artifact of how separate compilation is implemented.  The compiler and linker must love such separation, but I rarely find it to be useful.  And I'm apparently not the only one - languages in which separate compilation doesn't involve header files tend to unify interface and definition (Java, Ruby, C#, etc).

Final thought: I think we agree that there are *plenty* of opportunities to clean up JavaScriptCore's inline methods.  Lots of them are in the wrong file.  Lots more shouldn't even be inline.  And probably there are classes where (A) is more common than (B) and yet we have a bunch of large inline method bodies cluttering the interface.  Probably I'm guilty of some of that - I'm beginning to think that for most of the JSObject inline methods I added, I made the wrong call when I put them inside the class body.  And heck, that's only the beginning of the badness - we even have files that use incorrect indentation in namespaces, and goofy #include ordering, just because of legacy.  Of course we should fix the cases where the current style is obviously bad, and not just in those cases where the laws prescribed by the style guide would tell us to do so.  And of course the style guide should prohibit hijacking one class's header for another class's inline methods just because of circular dependencies - *inlines.h would be far better for this.  But to me the suggestion that we should simultaneously go further, and introduce additional rigidity into the style guide concerning the precise placement of inline method bodies within a header file, is drastic and unnecessary.

-F

> 
> Geoff
> _______________________________________________
> 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/20121105/1ffa68e3/attachment.html>