On Wed, May 27, 2015 at 01:44:26PM -0700, Junio C Hamano wrote:
> Paul Tan <[email protected]> writes:
>
> > @@ -17,6 +34,10 @@ struct am_state {
> > struct strbuf dir; /* state directory path */
> > int cur; /* current patch number */
> > int last; /* last patch number */
> > + struct strbuf msg; /* commit message */
> > + struct strbuf author_name; /* commit author's name */
> > + struct strbuf author_email; /* commit author's email */
> > + struct strbuf author_date; /* commit author's date */
> > int prec; /* number of digits in patch filename */
> > };
>
> I always get suspicious when structure fields are overly commented,
> wondering if it is a sign of naming fields poorly. All of the above
> fields look quite self-explanatory and I am not sure if it is worth
> having these comments, spending efforts to type SP many times to
> align them and all.
Just my 2 cents, but I think that grouping and overhead comments can
often make things more obvious. For example:
struct am_state {
/* state directory path */
struct strbuf dir;
/*
* current and last patch numbers; are these 1-indexed
* or 0-indexed?
*/
int cur;
int last;
struct strbuf author_name;
struct strbuf author_email;
struct strbuf author_date;
struct strbuf msg;
/* number of digits in patch filename */
int prec;
}
Note that by grouping "cur" and "last", we can discuss them as a group,
and the overhead comment gives us room to mention their shared
properties (my indexing question is a real question, btw, whose answer I
think would be useful to mention in a comment).
Likewise, by grouping the "msg" strbuf with the author information, it
becomes much more clear that they are all about the commit metadata
(though I would not be opposed to a comment above the whole block,
either).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
