Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
On Nov 5, 2012, at 9:27 PM, Geoffrey Garen 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 wor
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
>> (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.
Geoff
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
> In other words, Foo.h declaring class Foo can focus on interface over > implementation, even with a few short inline methods defined within the class > or right after it -- but larger inlines may be required for performance, and > their bodies can easily depend on headers not properly part of > Foo.h-as-interface, which should therefore not "bootleg" along via nested > #includes. Whereas FooInlines.h can nest its implementation dependencies > freely. Maybe we should amend again: (2) Adopt the convention that the following functions got into "*Inlines.h": (2a) Functions that cause circular header dependencies (2b) Functions that cause a header to #include another header that clients would not otherwise #include An example of (2a): JSValue::toWTFStringInline(). An example of (2b): Node::renderStyle(). Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
These rules generally look great. One suggestion:
On Nov 5, 2012, at 8:47 AM, Geoffrey Garen 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;
}
cheers,
G.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
Filip Pizlo wrote: On Nov 5, 2012, at 4:15 PM, Brendan Eich wrote: Implementation vs. interface distinctions can be fuzzy, but we've found it helpful to use this as a razor when shaving header files with inlines, before compile errors or compile time problems bite. I think that the total time spent fixing dependencies due to inline methods being in the main header is going to be less than the total time spent having to search through multiple headers when doing normal work. Oh, for sure -- it doesn't make sense to expend effort changing existing code just to match a vague rule about separating implementation from interface. Sorry if I seemed to suggest that. I started from a general "inline method implementations are not appropriate to put in interface definitions" assertion but allowed for "even with a few short inline methods defined within the class or right after it". I should have allowed for other reasons not to split out FooInline.h. As I've pointed out in past messages in this thread, we have classes where the best documentation of a method is the method's body - hence having the body inline is a big win for productivity. Agreed. This may have more to due with how JSC is laid out. I think the last time I encountered a need to put a method body outside of the main header was over a month ago, if not more. That's cool. If you end up needing all the relevant headers and the topological sort is straightforward, fewer files wins. /be ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
On Nov 5, 2012, at 4:15 PM, Brendan Eich wrote: > Geoffrey Garen wrote: >>> The proposed design requires adding a FooInlines.h include to source files >>> that use that function, when any function moves into FooInlines.h. This can >>> happen any time a function is made inline or when a short inline function >>> gets longer. >> >> You convinced me; I hadn't considered this burden. >> >> Le me amend: >> >>> (2) Adopt the convention that any class using "*Inlines.h" defines all >>> inline functions defined out-of-line in "*Inlines.h" >> >> To >> >> (2) Adopt the convention that nothing goes into "*Inlines.h" by default, and >> functions are added on demand as we discover functions that cause compile >> failures or long compile times. > > Hi Geoff, sorry to stick my nose in it but Mozilla uses WebKit code (YARR, > <3) so we care. The strong reason we've found beyond compile failures and > long compile times (or really, this is the underlying cause of either compile > failures or, alternatively, long compile times): inline method > implementations are not appropriate to put in interface definitions. > > In other words, Foo.h declaring class Foo can focus on interface over > implementation, even with a few short inline methods defined within the class > or right after it -- but larger inlines may be required for performance, and > their bodies can easily depend on headers not properly part of > Foo.h-as-interface, which should therefore not "bootleg" along via nested > #includes. Whereas FooInlines.h can nest its implementation dependencies > freely. > > Implementation vs. interface distinctions can be fuzzy, but we've found it > helpful to use this as a razor when shaving header files with inlines, before > compile errors or compile time problems bite. I think that the total time spent fixing dependencies due to inline methods being in the main header is going to be less than the total time spent having to search through multiple headers when doing normal work. As I've pointed out in past messages in this thread, we have classes where the best documentation of a method is the method's body - hence having the body inline is a big win for productivity. This may have more to due with how JSC is laid out. I think the last time I encountered a need to put a method body outside of the main header was over a month ago, if not more. -F > > /be >> >> Geoff >> >> ___ >> webkit-dev mailing list >> webkit-dev@lists.webkit.org >> http://lists.webkit.org/mailman/listinfo/webkit-dev > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
Geoffrey Garen wrote: The proposed design requires adding a FooInlines.h include to source files that use that function, when any function moves into FooInlines.h. This can happen any time a function is made inline or when a short inline function gets longer. You convinced me; I hadn't considered this burden. Le me amend: (2) Adopt the convention that any class using "*Inlines.h" defines all inline functions defined out-of-line in "*Inlines.h" To (2) Adopt the convention that nothing goes into "*Inlines.h" by default, and functions are added on demand as we discover functions that cause compile failures or long compile times. Hi Geoff, sorry to stick my nose in it but Mozilla uses WebKit code (YARR, <3) so we care. The strong reason we've found beyond compile failures and long compile times (or really, this is the underlying cause of either compile failures or, alternatively, long compile times): inline method implementations are not appropriate to put in interface definitions. In other words, Foo.h declaring class Foo can focus on interface over implementation, even with a few short inline methods defined within the class or right after it -- but larger inlines may be required for performance, and their bodies can easily depend on headers not properly part of Foo.h-as-interface, which should therefore not "bootleg" along via nested #includes. Whereas FooInlines.h can nest its implementation dependencies freely. Implementation vs. interface distinctions can be fuzzy, but we've found it helpful to use this as a razor when shaving header files with inlines, before compile errors or compile time problems bite. /be Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
> The proposed design requires adding a FooInlines.h include to source files > that use that function, when any function moves into FooInlines.h. This can > happen any time a function is made inline or when a short inline function > gets longer. You convinced me; I hadn't considered this burden. Le me amend: > (2) Adopt the convention that any class using "*Inlines.h" defines all inline > functions defined out-of-line in "*Inlines.h" To (2) Adopt the convention that nothing goes into "*Inlines.h" by default, and functions are added on demand as we discover functions that cause compile failures or long compile times. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
> Just so I understand, you're recommending we move functions like > Document::guardDeref() > > http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.h#L240 > > out of the class declaration, but leave them in the header file (e.g., > in Document.h). Correct. And, if we decided to add DocumentInlines.h, Document::guardDeref() would move to DocumentInlines.h. In either case, Document::canContainRangeEndPoint() would not change at all. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
On Nov 5, 2012, at 8:47 AM, Geoffrey Garen wrote: > (2) Adopt the convention that any class using "*Inlines.h" defines all inline > functions defined out-of-line in "*Inlines.h" This is a proposal I don’t agree with. Making a firm rule here here means that every larger inline function in one of these classes becomes a special case from a client point of view, where previously only functions actually affected by complex dependencies were special cases. The proposed design requires adding a FooInlines.h include to source files that use that function, when any function moves into FooInlines.h. This can happen any time a function is made inline or when a short inline function gets longer. This could affect any file that was using that function but was not previously using one of the other functions in FooInlines.h. That is a burden I would prefer the project not take on; it would make refactoring more difficult. Further, this proposal does not solve the problem of getting this wrong if we don’t actually try an appropriate build. As with today’s similar problems, include mistakes won’t be noticed if we are compiling with inlining off as we do on, say, Mac debug builds. > Choosing what to put in FooInlines.h based on header dependencies hurts my > brain. In the name of making it easier to write the headers correctly and slightly easier to find functions, this will make it much more common to have to include a separate header file, without a clear rule for when you need that include. The rule today is that we can just include the class’s header and use functions from that header, with a limited number of exceptions where we have to include another file. Putting more functions into separate files will spread this to other functions, making the problems with it worse. > I don't want to compute the set of all header dependencies when trying to > find a function definition. We should not compute header dependencies in cases like this. Instead we should look in all the source files, Foo.h, FooInlines.h, and Foo.cpp. Just as today we have to look in both Foo.h and Foo.cpp since we don’t know whether the function is inlined or not. > Also, I don't want to move things around when dependencies change. It would be good if we can accommodate you. But I don’t want to have to change all call sites when the complexity or inlining status of a member function changes. The rest of your proposal is something I agree on. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
Just so I understand, you're recommending we move functions like
Document::guardDeref()
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.h#L240
out of the class declaration, but leave them in the header file (e.g.,
in Document.h).
Thanks!
Adam
On Mon, Nov 5, 2012 at 8:47 AM, Geoffrey Garen wrote:
> Based on the feedback here, I would propose something like the following:
>
> (1) Globally rename "*InlineMethods.h" => "*Inlines.h".
>
> "*Inlines.h" is now the WebKit convention for how you put an inline function
> in a separate file.
>
> Update the style guide to say so.
>
> (2) Adopt the convention that any class using "*Inlines.h" defines all inline
> functions defined out-of-line in "*Inlines.h"
>
> Choosing what to put in FooInlines.h based on header dependencies hurts my
> brain. I don't want to compute the set of all header dependencies when trying
> to find a function definition. Also, I don't want to move things around when
> dependencies change.
>
> (3) Refactor to use "*Inlines.h" in the few cases in JSC where functions are
> in obviously unholy places.
>
> (4) Refactor to use "*Inlines.h" in the few cases where it would
> substantially improve compile times. (Do we know of such cases?)
>
> (5) Adopt the convention that any function that is not as trivial as "int x()
> { return m_x; }" moves out of the class declaration.
>
> This makes finding functions easier in the world of "*Inlines.h". Also, this
> puts class interface and data first, making things more readable.
>
> Update the style guide to say that "int x() { return m_x; }" should be one
> line.
>
> Update the style guide to say that more complex functions should not reside
> in the class declaration.
>
> How does that sound?
>
> Geoff
>
>
> On Nov 3, 2012, at 4:04 PM, Darin Adler wrote:
>
>> On Nov 3, 2012, at 10:09 AM, Mark Lam wrote:
>>
>>> 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.
>>
>> Inline functions that are kept inside vs. outside class definitions is a
>> separate issue from the in the same header file vs. in another header file.
>>
>> I agree that class definitions are often significantly easier to read if all
>> non-trivial function definitions are put outside the class. But requiring
>> that all such function definitions be put into a separate file seems unduly
>> awkward and heavy to me.
>>
>> Maciej stated the reasons we have created such files in the past; the intent
>> was not to put all inline functions in them. I would not want to create the
>> many new files this new convention would require.
>>
>>> 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;
>>>}
>>
>> The WebKit coding style document might formally require that as currently
>> written, but I think it’s a mistake and not consistent with the style we
>> actually use in WebKit coding.
>>
>>> I would propose updating the webkit coding style to allowing braces on the
>>> same line for the case of 1 line inline functions
>>
>> We should. It’s already part of WebKit coding style, even though it seems
>> not yet part of the formal WebKit coding style document.
>>
>> -- Darin
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org
>> http://lists.webkit.org/mailman/listinfo/webkit-dev
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
Sounds good to me, but I'll wait a bit to see if anyone else has more comments
before implementing. Thanks.
On Nov 5, 2012, at 8:47 AM, Geoffrey Garen wrote:
> Based on the feedback here, I would propose something like the following:
>
> (1) Globally rename "*InlineMethods.h" => "*Inlines.h".
>
> "*Inlines.h" is now the WebKit convention for how you put an inline function
> in a separate file.
>
> Update the style guide to say so.
>
> (2) Adopt the convention that any class using "*Inlines.h" defines all inline
> functions defined out-of-line in "*Inlines.h"
>
> Choosing what to put in FooInlines.h based on header dependencies hurts my
> brain. I don't want to compute the set of all header dependencies when trying
> to find a function definition. Also, I don't want to move things around when
> dependencies change.
>
> (3) Refactor to use "*Inlines.h" in the few cases in JSC where functions are
> in obviously unholy places.
>
> (4) Refactor to use "*Inlines.h" in the few cases where it would
> substantially improve compile times. (Do we know of such cases?)
>
> (5) Adopt the convention that any function that is not as trivial as "int x()
> { return m_x; }" moves out of the class declaration.
>
> This makes finding functions easier in the world of "*Inlines.h". Also, this
> puts class interface and data first, making things more readable.
>
> Update the style guide to say that "int x() { return m_x; }" should be one
> line.
>
> Update the style guide to say that more complex functions should not reside
> in the class declaration.
>
> How does that sound?
>
> Geoff
>
>
> On Nov 3, 2012, at 4:04 PM, Darin Adler wrote:
>
>> On Nov 3, 2012, at 10:09 AM, Mark Lam wrote:
>>
>>> 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.
>>
>> Inline functions that are kept inside vs. outside class definitions is a
>> separate issue from the in the same header file vs. in another header file.
>>
>> I agree that class definitions are often significantly easier to read if all
>> non-trivial function definitions are put outside the class. But requiring
>> that all such function definitions be put into a separate file seems unduly
>> awkward and heavy to me.
>>
>> Maciej stated the reasons we have created such files in the past; the intent
>> was not to put all inline functions in them. I would not want to create the
>> many new files this new convention would require.
>>
>>> 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;
>>> }
>>
>> The WebKit coding style document might formally require that as currently
>> written, but I think it’s a mistake and not consistent with the style we
>> actually use in WebKit coding.
>>
>>> I would propose updating the webkit coding style to allowing braces on the
>>> same line for the case of 1 line inline functions
>>
>> We should. It’s already part of WebKit coding style, even though it seems
>> not yet part of the formal WebKit coding style document.
>>
>> -- Darin
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org
>> http://lists.webkit.org/mailman/listinfo/webkit-dev
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
Based on the feedback here, I would propose something like the following:
(1) Globally rename "*InlineMethods.h" => "*Inlines.h".
"*Inlines.h" is now the WebKit convention for how you put an inline function in
a separate file.
Update the style guide to say so.
(2) Adopt the convention that any class using "*Inlines.h" defines all inline
functions defined out-of-line in "*Inlines.h"
Choosing what to put in FooInlines.h based on header dependencies hurts my
brain. I don't want to compute the set of all header dependencies when trying
to find a function definition. Also, I don't want to move things around when
dependencies change.
(3) Refactor to use "*Inlines.h" in the few cases in JSC where functions are in
obviously unholy places.
(4) Refactor to use "*Inlines.h" in the few cases where it would substantially
improve compile times. (Do we know of such cases?)
(5) Adopt the convention that any function that is not as trivial as "int x() {
return m_x; }" moves out of the class declaration.
This makes finding functions easier in the world of "*Inlines.h". Also, this
puts class interface and data first, making things more readable.
Update the style guide to say that "int x() { return m_x; }" should be one line.
Update the style guide to say that more complex functions should not reside in
the class declaration.
How does that sound?
Geoff
On Nov 3, 2012, at 4:04 PM, Darin Adler wrote:
> On Nov 3, 2012, at 10:09 AM, Mark Lam wrote:
>
>> 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.
>
> Inline functions that are kept inside vs. outside class definitions is a
> separate issue from the in the same header file vs. in another header file.
>
> I agree that class definitions are often significantly easier to read if all
> non-trivial function definitions are put outside the class. But requiring
> that all such function definitions be put into a separate file seems unduly
> awkward and heavy to me.
>
> Maciej stated the reasons we have created such files in the past; the intent
> was not to put all inline functions in them. I would not want to create the
> many new files this new convention would require.
>
>> 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;
>>}
>
> The WebKit coding style document might formally require that as currently
> written, but I think it’s a mistake and not consistent with the style we
> actually use in WebKit coding.
>
>> I would propose updating the webkit coding style to allowing braces on the
>> same line for the case of 1 line inline functions
>
> We should. It’s already part of WebKit coding style, even though it seems not
> yet part of the formal WebKit coding style document.
>
> -- Darin
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
On Nov 3, 2012, at 10:09 AM, Mark Lam wrote:
> 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.
Inline functions that are kept inside vs. outside class definitions is a
separate issue from the in the same header file vs. in another header file.
I agree that class definitions are often significantly easier to read if all
non-trivial function definitions are put outside the class. But requiring that
all such function definitions be put into a separate file seems unduly awkward
and heavy to me.
Maciej stated the reasons we have created such files in the past; the intent
was not to put all inline functions in them. I would not want to create the
many new files this new convention would require.
> 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;
> }
The WebKit coding style document might formally require that as currently
written, but I think it’s a mistake and not consistent with the style we
actually use in WebKit coding.
> I would propose updating the webkit coding style to allowing braces on the
> same line for the case of 1 line inline functions
We should. It’s already part of WebKit coding style, even though it seems not
yet part of the formal WebKit coding style document.
-- Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
On Nov 3, 2012, at 10:09 AM, Mark Lam wrote:
>
> On Nov 3, 2012, at 2:11 AM, Maciej Stachowiak wrote:
>> On Nov 3, 2012, at 1:48 AM, Mark Lam 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
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
On Nov 3, 2012, at 2:11 AM, Maciej Stachowiak wrote:
> On Nov 3, 2012, at 1:48 AM, Mark Lam 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?
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
o
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
On Nov 3, 2012, at 1:48 AM, Mark Lam wrote: > FYI, some time in the near future (maybe this weekend), I plan to do some > work to break inline methods in JavaScriptCore out of header files into their > own inline header files. > > Naming-wise, the existing JSC code has a few inline headers named …Inlines.h > and more named …InlinedMethods.h. On the WebCore side, the few that exists > there are named …InlineMethods.h. I have a preference for the …Inlines.h > convention because it is shorter and concisely communicates the intent. I > plan to rename all these inline headers to be …Inlines.h for consistency. > Does anyone object to my renaming the inline headers? > > Also, a lot of the existing inlined methods are 1 liners. I plan to leave > these in the original header file. Here's the convention I'm going to adopt: > > 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. > > I will only be breaking out the inline methods (per the above convention) in > the JSC sources. Apart from renaming the few WebCore inline headers, I won't > be looking into WebCore's placement of inline methods for now. > > If anyone has an opinion on this, please let me know asap. Thanks. 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. 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. 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. 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. So I'd like to understand why we want to do this. No objections on the rename though, Inlines.h is both more concise and more accurate than InlineMethods.h. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
On Nov 2, 2012, at 5:48 PM, Mark Lam wrote: > FYI, some time in the near future (maybe this weekend), I plan to do some > work to break inline methods in JavaScriptCore out of header files into their > own inline header files. > > Naming-wise, the existing JSC code has a few inline headers named …Inlines.h > and more named …InlinedMethods.h. On the WebCore side, the few that exists > there are named …InlineMethods.h. I have a preference for the …Inlines.h > convention because it is shorter and concisely communicates the intent. I > plan to rename all these inline headers to be …Inlines.h for consistency. > Does anyone object to my renaming the inline headers? > > Also, a lot of the existing inlined methods are 1 liners. I plan to leave > these in the original header file. Here's the convention I'm going to adopt: > > 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. I buy the intent here and agree that JavaScriptCore has too much stuff in headers. But I don't think that this rule - any inline method over one line should be in a separate file - is going to be a good idea in the long term. For example, I would find it much more difficult to reason about DFG::AbstractValue if I had to be looking at two different files to do it. Same for DFG::Node. Probably others as well. 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. So, for most of the inline methods with which I am familiar, I'd prefer if they stayed where they are now. -F > > I will only be breaking out the inline methods (per the above convention) in > the JSC sources. Apart from renaming the few WebCore inline headers, I won't > be looking into WebCore's placement of inline methods for now. > > If anyone has an opinion on this, please let me know asap. Thanks. > > Regards, > Mark > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
On Fri, Nov 2, 2012 at 5:48 PM, Mark Lam wrote: > FYI, some time in the near future (maybe this weekend), I plan to do some > work to break inline methods in JavaScriptCore out of header files into their > own inline header files. > > Naming-wise, the existing JSC code has a few inline headers named …Inlines.h > and more named …InlinedMethods.h. On the WebCore side, the few that exists > there are named …InlineMethods.h. I have a preference for the …Inlines.h > convention because it is shorter and concisely communicates the intent. I > plan to rename all these inline headers to be …Inlines.h for consistency. > Does anyone object to my renaming the inline headers? I support this change, and I also prefer Inlines.h since "method" isn't a C/C++ terminology. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] …Inlines.h vs …InlineMethods.h
My understanding was that *Inlines/*InlineMethods were more about limiting includes in the main header. Maybe that's just a happy side effect. :) I also prefer the *Inlines naming. :) On Fri, Nov 2, 2012 at 5:48 PM, Mark Lam wrote: > FYI, some time in the near future (maybe this weekend), I plan to do some > work to break inline methods in JavaScriptCore out of header files into their > own inline header files. > > Naming-wise, the existing JSC code has a few inline headers named …Inlines.h > and more named …InlinedMethods.h. On the WebCore side, the few that exists > there are named …InlineMethods.h. I have a preference for the …Inlines.h > convention because it is shorter and concisely communicates the intent. I > plan to rename all these inline headers to be …Inlines.h for consistency. > Does anyone object to my renaming the inline headers? > > Also, a lot of the existing inlined methods are 1 liners. I plan to leave > these in the original header file. Here's the convention I'm going to adopt: > > 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. > > I will only be breaking out the inline methods (per the above convention) in > the JSC sources. Apart from renaming the few WebCore inline headers, I won't > be looking into WebCore's placement of inline methods for now. > > If anyone has an opinion on this, please let me know asap. Thanks. > > Regards, > Mark > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] …Inlines.h vs …InlineMethods.h
FYI, some time in the near future (maybe this weekend), I plan to do some work to break inline methods in JavaScriptCore out of header files into their own inline header files. Naming-wise, the existing JSC code has a few inline headers named …Inlines.h and more named …InlinedMethods.h. On the WebCore side, the few that exists there are named …InlineMethods.h. I have a preference for the …Inlines.h convention because it is shorter and concisely communicates the intent. I plan to rename all these inline headers to be …Inlines.h for consistency. Does anyone object to my renaming the inline headers? Also, a lot of the existing inlined methods are 1 liners. I plan to leave these in the original header file. Here's the convention I'm going to adopt: 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. I will only be breaking out the inline methods (per the above convention) in the JSC sources. Apart from renaming the few WebCore inline headers, I won't be looking into WebCore's placement of inline methods for now. If anyone has an opinion on this, please let me know asap. Thanks. Regards, Mark ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
