Re: CVS update: subversion/subversion/libsvn_delta delta.h text_delta.c vdelta.c

In a few cases I think the diffs showed things as changes when I
hadn't really thought of them that way while making the change. In
particular, the stuff related to current_match_*.

> Please, don't declare two vars in a single statement. You save
> space, yes, at the expense of readability.

I don't think it really harms readability. If other developers feel
the same way Branko does, then I'm happy to adjust my style for
Subversion code, but I can't really tell whether that's the case from
looking at the existing Subversion code. There aren't very many
examples of two variables of the same type being declared in the same
place.

> Also IMNSHO all pointers should be initialized when declared. You'll
> have some redundant initializations, which the compiler will elide
> when optimizing; and you'll catch bugs from uninitialized pointer
> usage much sooner.

I have the opposite opinion. The compiler will warn you about
variables being used when they might not be initialized, and you
defeat that warning if you do "shotgun initialization" like you say.

> - ++here;
> + here++;

> What's the point of this change? Looks just a bit gratuitous to me ...

Well, everywhere else in the Subversion code that I've read uses
"foo++". Yes, it was a bit pointless; I did it mostly without
thinking.

> Another thing: I notice you mixed cosmetic (formatting, typos)
> changes and "real" changes in the same commit. I advise against
> it. It makes it harder for other people to understand the changes.

I try to avoid such mixing myself, but in this case I felt like since
I was reorganizing the code, the diffs wouldn't be especially useful
anyway. (Especially since they don't use -w.)
Received on Sat Oct 21 14:36:10 2006