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

[Filip Pizlo]

On Nov 3, 2012, at 10:09 AM, Mark Lam <mark.lam at apple.com> wrote:

> 
> On Nov 3, 2012, at 2:11 AM, Maciej Stachowiak <mjs at apple.com> wrote:
>> On Nov 3, 2012, at 1:48 AM, Mark Lam <mark.lam at apple.com> wrote:
>>> 1. If the inline method is a 1 liner, then leave it in the original header file.
>>> 2. If it takes more than 1 line, then move it into a corresponding inline header file.
>> 
>> What is the intended benefit of more thoroughly splitting out inline methods? The two reasons I know to do it are:
>> 
>> (a) In some cases, it is required to avoid a circular dependency in header includes (where two classes have inline member functions that each depend on the other class).
>> (b) If inline methods require pulling in additional includes, but not all files including the main header need their definitions, it can speed up compile times.
>> 
>> It does not seem to me like a cutoff of 1 line vs more than 1 line is in very good alignment with reasons (a) and (b) to split inline member functions into a separate header.
>> 
>> So I'd like to understand why we want to do this.
> 
> 
> Avoiding circular dependency issues is the reason why I looked into inline headers in the first place.  Currently, there are inline functions that are put in different header files (not an intuitive place and requires a grep to find) just to avoid the circular dependency.  I would like to make it more intuitive to know where to find functions without having to grep i.e. would like to have a cleaner model of how source code is packaged (and present less mental clutter for the engineer working with the code).  
> 
> My reasons for proposing the 1 line vs multi-line convention are:
> 
> 1. to keep class definitions more compact
>    - so you can see more of the entire class (not that this is always possible anyway).
>    - being able to see the entire class (when possible) can yield useful information about the shape of the class i.e. let's me see the architecture / design, not just the implementation.
>    - having lots of inline functions bodies inside the class definition bloats the class significantly and makes it harder to see the shape. (again, a mental clutter issue).  Of course, if you have editor tools that can hide the function bodies, this is not an issue.
> 
> 2. to provide some consistency to the styling as to where to expect to find inline functions (i.e. inline methods are in Inlines.h).
> 
> However, it would be undue burden to have to move all trivial inline functions like 1 line setter/getters to an Inlines.h file.  By 1 liners, I mean something like:
> 
>     bool isFinished { return m_isFinished; }
> 
> … but now am realizing that this is not allowed by the webkit coding style, which instead requires:
> 
>     bool isFinished
>     {
>         return m_isFinished;
>     } 
> 
> Anyway, I proposed this 1 liner convention (with assuming it is really a 1 liner, not a 4 liner) so that we don't have to move trivial inlines to the inline header.  Having a mechanical convention for this also makes it so that the engineer will not have to make a value judgement on the compactness issue when writing the code and can focus on the implementation (again less mental clutter).
> 
>> The main downside is that you now have to keep track of whether you need the second header when you use a class, and if you forget, you may not see the failure until late in the build process.
> 
> Agreed.  It's unfortunate but true.
> 
>> It also seems like it would create friction when changing inline methods, since taking it over or under the one-line cutoff would now require you to move it to a different file.
> 
> Also agreed, but it would prevent slop which eventually leads to the class definition bloating up … which is what I was trying to avoid in the first place.
> 
> As for Filip's argument:
> 
>> The general challenge here is that many of the methods have semantics that are best deduced by not only reading their names but also looking at their contents.
> 
> I'm not sure I agree with this point as much.  I agree with the "sometimes function names are not adequate" part, but wouldn't it be equally meaningful if you were reading effectively the same body of code (the inline functions) in the Inlines.h file instead?  Am I not seeing something here?

- You're likely to want to place any comments describing the behavior of the method in Foo.h rather than FooInlines.h.

- All field declarations are in Foo.h.

- Some method bodies will be in Foo.h (if they're short enough).

Hence, for many classes, your proposal will mean that I'll have to have one extra file open.

This gets even more frustrating if a class also has out-of-line methods.  Then I have to look at Foo.h, Foo.cpp, and FooInlines.h.  It's a bit much for my tastes.

That being said, I have no objection to introducing FooInlines.h in cases where we currently place the inline method implementations in a different class's header due to dependencies.  That quirk needs to be fixed!

> 
> That said, since the webkit coding style prohibits true 1 line inline functions, I feel less enthusiastic about my proposal as it would either:
> 1. automatically necessitate moving a lot of functions (all the 1 liners that are expressed as 4 liners), or
> 2. not give me the benefit I want (i.e. to improve my chances of seeing the entire class at a glance) because lots of 4 liners will still bloat the class definition. 
> 
> I would propose updating the webkit coding style to allowing braces on the same line for the case of 1 line inline functions because that is a good thing (code compactness), but that is a separate subject altogether.  As for my 1 line vs multi-line convention, for now, I'm inclined to go with dropping it, and leaving the decision to move a function to Inlines.h or not to the discretion of the engineer working on the code.

 I was under the impression that WebKit style permits true 1 line inline functions.  I use them all the time.  So that shouldn't be an argument against your proposal.

> 
> Any opinions / thoughts?  Thanks.

Just consider for a moment classes like the following: DFG::Node, DFG::AbstractValue, and JSObject.  These classes come to mind because they have behavior for which one must employ some mental gymnastics, and so I find myself reading the code for those classes a lot.

DFG::Node: This is a glorified struct, with encapsulation used to capture the fact that the class encodes state in a peculiar way (example: m_opInfo, m_opInfo2 are reused to mean many different things). Most methods are in the 2-3 line range.  If you moved some of it into a separate class, the class becomes strictly less hackable.  Without your change, if I want to add a "property" to Node, I just have to hack in a few short methods in DFGNode.h.  With your change, I end up doing more work: first I add declarations in DFGNode.h and then I would add bodies in DFGNodeInlines.h.  I don't want to do more work.  Especially where in this case, doing more work would literally accomplish nothing.  That is very much at the heart of my argument.

DFG::AbstractValue: This is also a glorified struct, but in a different way.  Its fields are meant to be queried directly, and most of the documentation of how the struct behaves is in comments associated with those fields.  But it also has very interesting methods.  Without your change, I can always find what I'm looking for by just opening DFGAbstractValue.h and skimming through it - reminding myself (a) what methods it has, (b) what those methods do, (c) what fields it has, (d) what the comments for those fields say.  With your change, I'd have to open two files and skim both of them.

JSObject: Right now all of this class's stuff is in JSObject.h and JSObject.cpp.  We don't want to change the methods whose bodies live in JSObject.cpp into inline methods.  Without your change, I already have two places where I need to look to understand JSObject: I need to look at JSObject.h, and JSObject.cpp.  Your change would introduce a third place that I would have to look - JSObjectInlines.h.  Again, that's more work.

In short, my objection to taking all >1line methods and putting them into a new, previously non-existant header file, is that it would mean more work whenever I'm trying to find something, and also more work when I'm trying to add new code.  Since we spend nearly all of our time doing one of those two things, I'd rather that we didn't make non-functional changes that increased the latency of those tasks.

-F

> 
> Regards,
> Mark
> 
>