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 wrote: >> On Mon, Jan 31, 2011 at 4:54 PM, Aaron Boodman 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
Re: [webkit-dev] Fwd: coding style and comments
On Mon, Jan 31, 2011 at 5:48 PM, Aaron Boodman wrote: > void doA() { > // We don't need to frobulate here because doC() already did that. > } I meant for doA() to call doB() obviously. - 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 5:23 PM, Ryosuke Niwa wrote: > On Mon, Jan 31, 2011 at 4:54 PM, Aaron Boodman 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 Mon, Jan 31, 2011 at 4:54 PM, Aaron Boodman 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 12:47 AM, Ryosuke Niwa 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
2011/1/31 Konstantin Tokarev > > 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
31.01.2011, 11:47, "Ryosuke Niwa" : > 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