On 2013/09/26 10:51:34, janek wrote:
> It does not belong in the code. It might belong in the issue
tracker,
> perhaps in the commit message. Just as a record of what went wrong.
> But the code, as it is written, does not offer a temptation of
changing
> it back to the buggy previous version. It was not more elegant or
> obvious or clearer.
Uh, David, thanks for this explanation, but i'm afraid it was not
needed. If the code doesn't need a comment, just say so.
Now, speaking in general: i don't understand that when Mike submits a
patch, you often complain that it's not commented well enough, but
when I ask you for comments, you usually say "this doesn't need any
comment".
Apparently both you and Mike consider what you write to be
self-explanatory.
git blame lily/lexer.ll|grep "//\|/\*"|wc -l
146
git blame lily/lexer.ll|grep "//\|/\*"|grep Kastrup|wc -l
120
git blame lily/lexer.ll|wc -l
1382
git blame lily/lexer.ll|grep Kastrup|wc -l
677
Are you sure that your characterization is fair? I'm responsible for
less than half the lines in the lexer, but for more than 80% of the
lines starting a comment in the lexer.
It's a bit hard to do the same kind of check on a typical C++ file
since superficially most lines are blamed on whoever ran the indenting
tool last. lily/lexer.ll is not automatically indented, so it's
somewhat more reliable.
When my typical complaint with a commit from Mike focuses on the
_number_ of comments, it's usually because there are about 10 lines of
comments in 5000 lines of code (which seems indeed out of proportion
based purely on the numbers, particularly when the whole _framework_
put together in those 5000 lines is non-trivial, opaque and mostly
non-discoverable), and half of the comments are wrong.
Now statistics can definitely be misleading as indeed a comment can be
worthless and/or distracting and always is in danger of running out of
sync with the code. So it needs to have value _separate_ from the
code in order to be worth it. If it repeats the story spelled out in
the code (and this is what you ask for here), it's useless. If it
_summarizes_ a passage of code, it's already separately useful. If it
puts code into context, it's _quite_ useful.
Maybe you are right that for an experienced developer Mike's code
isn't self-explanatory, and your code is. All i know is that for me
neither is self-explanatory.
So you say that a pattern written starting with
[^.*|...
does not tell you that it is supposed to match something not starting
with a . * | and whatever else is listed here. That's a matter of not
knowing Flex and/or regular expressions, not a matter of not being
able to follow the logic of the code.
That's like asking for a comment like
x += 2; // increase x by 2
which is not helpful.
Of course, i'm not an experienced
programmer. But then, i've been working on LilyPond since 3 years
now, and that seems a pretty long time.
I am sorry that (apparently) you don't care whether i can understand
your patches easily or not.
You will understand a patch to the lexer _only_ if you know the
language the lexer is written in. A code review that has to rely on
_comments_ in order to "understand" every facet of the code (rather
than larger contexts and/or its design) is basically useless as any
divergence between code and comment can't be detected.
Also, from my perspective, it seems that
you consider yourself to be always right:
"i think this deserves a regtest" "no, it doesn't"
If you consider "no, it doesn't" as the same as "a regtest in that
area should at least cover that and that cases. Would you like to
propose one?", sure.
Can you point out to any case where I flat out stated that a regtest
would not be warranted?
"i think this deserves a comment" "no, it doesn't"
Can you please propose a comment that does not
a) talk about a state that's not actually in the code and won't get
there accidentally
b) is not _immediately_ _literally_ obvious from the code for anybody
knowing the language that something is written in?
Comments should increase the signal/noise ratio of a program, not
decrease it. They need to tell the story that is _not_ explicitly
coded.
I suppose that you're not doing this intentionally. But it drives
me crazy anyway. Anyway, it seems that when i try to review your
patches the first and foremost result is that we both loose time,
Does that mean that I can save myself the effort to _explain_ my
decisions since you would consider it a loss of time to try
understanding them?
You _are_ aware that it takes much less effort to put in a useless
comment rather than explain why it wouldn't help in a particular case?
So obviously my main aim is not to reduce the amount of work.
so i suppose that i should stop reviewing your patches
Depends on your goals.
https://codereview.appspot.com/13256053/
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
