On Nov 5, 2012, at 9:27 PM, Geoffrey Garen <[email protected]> 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
> [email protected]
> http://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________
webkit-dev mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-dev
