Fri Sep 2 08:17:00 GMT 2011
On Aug 29 11:19, Steven Abner wrote:
> On Aug 29, 2011, at 10:32 AM, Corinna Vinschen wrote:
> > resend the full patch relative to CVS
> One combo patch, hold the fries. :)
Thanks. I had a first look, and while it seems it's doing what it's
supposed to do, I'm not entirely happy with the patch as is:
- You changed validate_structure to return some value, but you neglected
to add a comment to explain the innocent observer what is returned and
especially why you return it.
- Why did you add a `day' variable which is identical to tim_p->tm_mday
at the start, but is not changed later on when tim_p->tm_mday is
changed? What is the difference between day and tim_p->tm_mday?
There should be a comment to explain that.
- You ripped off the entire code of the block starting at
if (y == tz->__tzyear || __tzcalc_limits (y))
That's fine, but the old code contained a couple of comments to explain
what it's doing. Your new code block contains not even a single comment.
Please add comments. The code is *not* self-explanatory, except, maybe,
for the guy who wrote it. And even then you may be surprised when you
try to understand your own code in 10 years from now. BTDT.
- The patch contains a couple of gratuitous changes which only change
the coding style for no apparent reason. For instance whitespace
changes or a change from a while into a do...while loop without
changing the semantics.
Other than that, the changes look ok. If you could just change the
Cygwin Project Co-Leader
More information about the Newlib