[webkit-dev] Fwd: coding style and comments
On Fri, Jan 28, 2011 at 10:21 AM, Peter Kasting pkast...@google.com wrote: On Fri, Jan 28, 2011 at 10:14 AM, Alexey Proskuryakov a...@webkit.orgwrote: Do you have any specific mechanism in mind for keeping global comments accurate? No more than I have for keeping API usage or function implementations correct; that is, if we have comments, they must be as important to reviewers and patch authors as the code is. How can we ensure that all comments are up to do date? For example, suppose function A calls B, and B calls C. Then in the call site of A, I comment Because A does X, we do Y. Now suppose for the moment that the behavior X of A is implemented by C. We then come back and modify C, thereby modifying the behavior X of A to X'. We suddenly have a wrong comment in the call site of A and we need to fix it! But how do we know that if the patch only changed one line in C? - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
31.01.2011, 11:47, Ryosuke Niwa rn...@webkit.org: How can we ensure that all comments are up to do date? For example, suppose function A calls B, and B calls C. Then in the call site of A, I comment Because A does X, we do Y. Now suppose for the moment that the behavior X of A is implemented by C. We then come back and modify C, thereby modifying the behavior X of A to X'. We suddenly have a wrong comment in the call site of A and we need to fix it! But how do we know that if the patch only changed one line in C? - Ryosuke You can document A as function calling B, B as function calling C, and keep documentation of C up to date when it's behavior changes But if A is API function, its behavior (what C does) should be described in place anyway -- Regards, Konstantin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
2011/1/31 Konstantin Tokarev annu...@yandex.ru You can document A as function calling B, B as function calling C, and keep documentation of C up to date when it's behavior changes I don't see how that can substitute my comment that Because A does X, do Y. Saying do Y because we call A isn't useful at all here. But if A is API function, its behavior (what C does) should be described in place anyway Okay, let me give you a more concrete example. In this case: http://trac.webkit.org/browser/trunk/WebCore/editing/IndentOutdentCommand.cpp?rev=41034#L79 we're working around an issue in moveParagraph: http://trac.webkit.org/browser/trunk/WebCore/editing/CompositeEditCommand.cpp?rev=41034#L737 Now, moveParagraph is a very complicated function that does a million of things depending on the structure of DOM and 5 arguments, and it's mutually recursive with 2 other editing commands ReplaceSelectionCommand and DeleteSelectionCommand, both of which depend on many other editing commands and the rest of WebCore. As a result, it's virtually impossible to describe the complete behavior of moveParagraph. And if we changed any one line in WebCore/editing, there is a significant chance that we're changing the behavior of either ReplaceSelectionCommand or DeleteSelectionCommand and subsequently, moveParagraph. This is just a tip of an iceberg. I can give you hundreds of examples where you can't describe the behavior of a function completely, and yet you need to be concerned with it in call sites. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
On Mon, Jan 31, 2011 at 12:47 AM, Ryosuke Niwa rn...@webkit.org wrote: How can we ensure that all comments are up to do date? For example, suppose function A calls B, and B calls C. Then in the call site of A, I comment Because A does X, we do Y. Now suppose for the moment that the behavior X of A is implemented by C. We then come back and modify C, thereby modifying the behavior X of A to X'. We suddenly have a wrong comment in the call site of A and we need to fix it! But how do we know that if the patch only changed one line in C? It seems like the one line patch to C just broke A. It had a dependency on the behavior of C that was worth documenting. Now you have changed C and the behavior of A is probably wrong (or at least wasteful). If you had the comment, at least a grep of the source would have found the dependency and alerted you that it was worth looking at this call site. - a ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
On Mon, Jan 31, 2011 at 4:54 PM, Aaron Boodman a...@chromium.org wrote: It seems like the one line patch to C just broke A. It had a dependency on the behavior of C that was worth documenting. Now you have changed C and the behavior of A is probably wrong (or at least wasteful). Not necessarily. X' might be better a behavior and Y might no longer be needed because of that. If you had the comment, at least a grep of the source would have found the dependency and alerted you that it was worth looking at this call site. I don't think so. How do I know that modifying C would have changed the behavior of A? This was a very simple example with only one indirection, namely, B. But in the example I posted earlier (moveParagraph), a function calls hundreds of thousands of functions and it's virtually impossible even to enumerate all functions depended by the function. Yet, we must worry about the side-effects caused by the function in a call site. And we have tons of functions like this in editing because of the nature of what it does. So I insist on my point that keeping comments up-to-date is really hard if not impossible. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
On Mon, Jan 31, 2011 at 5:23 PM, Ryosuke Niwa rn...@webkit.org wrote:
On Mon, Jan 31, 2011 at 4:54 PM, Aaron Boodman a...@chromium.org wrote:
It seems like the one line patch to C just broke A. It had a
dependency on the behavior of C that was worth documenting. Now you
have changed C and the behavior of A is probably wrong (or at least
wasteful).
Not necessarily. X' might be better a behavior and Y might no longer be
needed because of that.
That is exactly my point. Either way the one line change leaves orphan
code. Having a comment just improves your chances of finding it
(assuming the comment references the depended-upon code).
If you had the comment, at least a grep of the source would have found the
dependency and alerted you that it was worth looking at this call site.
I don't think so. How do I know that modifying C would have changed the
behavior of A?
To be specific, I'm talking about this situation:
void doA() {
// We don't need to frobulate here because doC() already did that.
}
void doB() {
doC();
}
void doC() {
.. complicated stuff ...
}
Now if someone comes along and changes doC, they will probably grep
for the call sites to update them. That grep would find the comment in
doA() too, bringing attention to the tricky indirect relationship.
Without the comment, the relationship would be harder to find.
This was a very simple example with only one indirection,
namely, B. But in the example I posted earlier (moveParagraph), a function
calls hundreds of thousands of functions and it's virtually impossible even
to enumerate all functions depended by the function. Yet, we must worry
about the side-effects caused by the function in a call site.
And we have tons of functions like this in editing because of the nature of
what it does. So I insist on my point that keeping comments up-to-date is
really hard if not impossible.
I'm not suggesting there should be a rule that all side-effects are
commented, that is clearly ridiculous. It is a matter of judgement to
determine when something warrants explanation. Typically it would be
when something is working in a way that is surprising or unexpected.
For example, moveParagraph() ends up calling lots of functions that
have lots of side-effects. Yet those shouldn't be surprising to
someone familiar with how editing works at a high level (note: this is
where class-level comments are super useful), so they shouldn't
require a comment.
- a
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fwd: coding style and comments
On Jan 31, 2011, at 5:48 PM, Aaron Boodman wrote:
On Mon, Jan 31, 2011 at 5:23 PM, Ryosuke Niwa rn...@webkit.org wrote:
On Mon, Jan 31, 2011 at 4:54 PM, Aaron Boodman a...@chromium.org wrote:
It seems like the one line patch to C just broke A. It had a
dependency on the behavior of C that was worth documenting. Now you
have changed C and the behavior of A is probably wrong (or at least
wasteful).
Not necessarily. X' might be better a behavior and Y might no longer be
needed because of that.
That is exactly my point. Either way the one line change leaves orphan
code. Having a comment just improves your chances of finding it
(assuming the comment references the depended-upon code).
If you had the comment, at least a grep of the source would have found the
dependency and alerted you that it was worth looking at this call site.
I don't think so. How do I know that modifying C would have changed the
behavior of A?
To be specific, I'm talking about this situation:
void doA() {
// We don't need to frobulate here because doC() already did that.
}
If previous frobulation is a precondition of doA, that's better expressed as an
ASSERT than as a comment. That way, if someone breaks the precondition, they'll
find out when they test, whether or not they think to search for related
comments.
I 3 asserts as a way to document preconditions.
Regards,
Maciej
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
