mktime()

[Corinna Vinschen]

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
above, please.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat