Re: [PATCH 2/8] xdl_change_compact(): clarify code

From: Stefan Beller <sbeller@google.com>
Date: Aug 3, 2016, 23:50 UTC
Message-ID: <CAGZ79kbyCRDTt4u+Fje819mNZZf3GkZtYVurwOMPXRfXqO-YEw@mail.gmail.com>
In-Reply-To: <57715dee-ca73-c1bb-ee79-2813d7873649@alum.mit.edu>

On Wed, Aug 3, 2016 at 4:14 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Show 9 quoted lines

> On 08/04/2016 12:11 AM, Stefan Beller wrote:
>> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> Write things out a bit longer but less cryptically. Add more comments.
>>
>> By less cryptic you mean in Git coding style ;)
>> The original author (do we want to cc Davido?) may disagree.
>
> Davide hasn't contributed since 2008 and libxdiff is not being
> developed, so I didn't think he'd be interested.

ok.

>
> Yes, tastes certainly differ. If more people like the old version
> better, I will gnash my teeth and undo these "clarification" patches.

I was not asking for undoing these, but giving short cryptic answers myself. ;) While I agree the variable names are way better than before, the use of while instead of for (and then doing another final ++ after the loop) extended some one liners to about 5. I am totally fine with that as they are easier to read for me as I understand them as Git style, hence easier to read.

There may be old timers (who have knowledge of C from other projects), that would prefer the style as before:

e.g.

-               start = i;
-               for (i++; rchg[i]; i++);
-               for (; rchgo[io]; io++);
+               start = i++;
+
+               while (rchg[i])
+                       i++;
+
+               while (rchgo[io])
+                      io++;

This doesn't change variable names, but it only transforms a for loop with no body in a more readable structure of while loops separated by white space. So for such a chunk I could imagine people arguing about adding lines of code (which is valuable screen real estate) for only slight gain if any. I am not one of these.

> I
> mean, what's not to like about variable names like "grpsiz" and "ixref"?

faster typng ;)

Show 23 quoted lines

>
>>> +
>>> +                       /*
>>> +                        * Are there any blank lines that could appear as the last
>>> +                        * line of this group?
>>> +                        */
>>
>> IIRC this comment is not quite correct as this 'only' counts the number of
>> blank lines within the forward shifting section, i.e. in the movable space.
>>
>> Later we use it as a boolean indicator (whether or not it is equal to 0)
>> to see if we can do better.
>>
>> Any other change in code and comments looks good to me, but this stood out
>> like a sore thumb. (Probably the old heuristic as a whole stood out, but the
>> comment here specifically sounds /wrong/ to me in this place. How can
>> a question document a variable? I'd rather expect a question comment
>> to ease the understanding of a condition)
>
> I don't understand your objection. A blank line can appear as the last
> line of the group if and only if it is within the shift range ("movable
> space") of the group, right? So it seems like our formulations are
> equivalent.

Sure, e.g. in 0fe5043da (2016-06-17, dir_iterator: new API for iterating over a directory tree), struct dir_iterator_int we have a member

    struct dir_iterator_int {
...
         /*
         * The number of levels currently on the stack. This is always
         * at least 1, because when it becomes zero the iteration is
         * ended and this struct is freed.
         */
         size_t levels_nr;
...
};

you could have written that comment as

    /* How many levels do we have to free? */

but that would be misleading the same way as here.

I think a comment should carry useful information that is not obvious from the code. So in this comment we want to convey the message that we need to count blank lines to apply a heuristic later on. Maybe

  /* Number of blank lines in the sliding area of the group */

as that

* states the actual thing we do
* doesn't hint at one particular intended use case later on
* it assumes you know what a "sliding area" is though.

I think what triggered me on questioning this comment, was the fact that it is a question as we rarely have comments stated as questions.

>
> Since the variable is used as a boolean, it seemed natural to document
> it by stating the question that the true/false value is the answer to.

Oh I see. Another example (that maybe looks constructed) is the comment of S_IFGITLINK in cache.h which is not "Is this entry a submodule?" but rather some sentence of what a git link actually is. (though very short)

>
> If you have a concrete suggestion for a better comment, please let me know.

I'd go with the imperative form,

 /* Number of blank lines in the sliding area of the group */

if that makes sense to you?

Sorry for the bike shedding and not focusing on the real issues,

Stefan

>
> Michael
>