Path: utzoo!utgpu!news-server.csri.toronto.edu!rutgers!mailrus!wuarchive!usc!apple!uokmax!munnari.oz.au!mel.dit.csiro.au!yarra!melba.bby.oz.au!leo!zvs
From: zvs@bby.oz.au (Zev Sero)
Newsgroups: comp.lang.c
Subject: Re: Where is strftime(.....)?
Message-ID: <1990Aug16.020847.25321@melba.bby.oz.au>
Date: 16 Aug 90 02:08:47 GMT
References: <2658@xenon.stgt.sub.org>
Sender: news@melba.bby.oz.au
Organization: Burdett, Buckeridge and Young Ltd.
Lines: 165
In-Reply-To: alf@xenon.stgt.sub.org's message of 12 Aug 90 15:03:47 GMT

In article <2658@xenon.stgt.sub.org> alf@xenon.stgt.sub.org (Ingo Feulner)
supplies a strftime() function.

A few problems:

  size_t
  strftime(buf, max, fmt, tm)
  char *buf;
  size_t max;
  const char *fmt;
  const struct tm *tm;
  {

All very well, but nowhere in the function is max ever checked!
_______
      short i = 0;
      buf[i] = 0;
      while (*fmt) {
  	  char *ptr;
>	  i += strlen(buf + i);
	  if (*fmt != '%') {
	      buf[i++] = *fmt++;
	      continue;
	  }

What on earth is the function of the line indicated?  I can't imagine
what the author is *trying* to do, but here is how it will actually
work (or rather, not work).

Imagine the following setup:
buf is passed to strftime() containing "Some message",
and fmt contains "Date: %A %d %B %Y".
  
The first thing strftime() does is 
      short i = 0;
      buf[i] = 0;
so buf now contains 
{'\0','o','m','e',' ','m','e','s','s','a','g','e','\0'}.
Now *fmt is 'D', so we enter the loop.
      while (*fmt) {
	  i += strlen(buf + i);
strlen (buf + 0) is 0, so i still equals 0.  
	  if (*fmt != '%') {
	      buf[i++] = *fmt++;
	      continue;
buf now contains
{'D','o','m','e',' ','m','e','s','s','a','g','e','\0'},
i is 1, and fmt points to "ate: %A %d %B %Y".
Now *fmt is 'a', so we enter the loop.
      while (*fmt) {
	  i += strlen(buf + i);
strlen (buf + 1) is 11, so i is now 12!
_________
	case '%':
	    strcpy(ptr, "%");
	    break;
And many similar cases.  Surely 
	    *ptr = '%';
would be much more efficient!
_________
	case 'I':   /*  hour as integer 01-12       */
	    sprintf(ptr, "%02d", (tm->tm_hour % 12) + 1);
	    break;
Say what???!!!  This should, of course, be something like
	    sprintf(ptr, "%02d",
		 tm->tm_hour % 12 ? tm->tm_hour % 12 : 12);
_________
	case 'j':   /*  day of year as int 001-366  */
	    sprintf(ptr, "%03d", tm->tm_yday);
	    break;
	case 'm':   /*  month as integer 01-12      */
	    sprintf(ptr, "%02d", tm->tm_mon);
	    break;
Must add 1 in both cases
_________
	case 'p':   /*  'AM' or 'PM'                */
	    if (tm->tm_hour >= 12) {
		strcpy(ptr, "PM");
	    } else {
		strcpy(ptr, "AM");
	    }
	    break;
Would be better as
            *ptr++ = tm->tm_hour < 12 ? 'A' : 'P';
	    *ptr = 'M';
_________
	case 'U':   /*  week of year as int 00-53, regard sunday as first day in week   */
	    {
		int bdiw = tm->tm_yday - tm->tm_wday;	/* beginning of week */
		if (bdiw < 0)
		    bdiw = 0;
		else
		    bdiw = bdiw / 7;
		sprintf(ptr, "%02d", bdiw);
	    }
	    break;

This will only work if the year begins on a Sunday.  Consider what
happens if the year begins on a Monday.  Sunday 7 Jan has a yday of 6,
and a wday of 0.  (6 - 0) / 7  gives an answer of 0.  The correct
answer is 1.  This code should be:
	case 'U':  /* Sunday week of the year */
		sprintf (ptr, "%02d", (tm->yday + 6 - tm->wday) / 7);
		break;
_________
	case 'W':   /*  day of week as int 0-6, monday == 0 */
	    {
		int dowmon = tm->tm_wday - 1;
		if (dowmon < 0)
		    dowmon += 7;
		sprintf(ptr, "%d", dowmon);
	    }
	    break;
This is wrong.  %W is `the Monday week of the year, from 00', i.e. it
is exactly the same as %U except that the week is considered to run
from Monday to Sunday.  The correct formula is:
	case 'W':  /* Monday week of the year */
	    sprintf (ptr, "%02d",
	        (tm->yday + 6 - (tm->wday ? tm->wday - 1 : 6)) / 7);
________
	case 'x':   /*  the locale's default rep for date   */
	    sprintf(ptr, "%s %s %02d",
		AbDow[tm->tm_wday], AbMonth[tm->tm_mon], tm->tm_mday
	    );
	    break;
	case 'X':   /*  the locale's default rep for time   */
	    sprintf(ptr, "%02d:%02d:%02d",
		tm->tm_hour, tm->tm_min, tm->tm_sec
	    );
	    break;
These are obviously not fully implemented.  Fair enough, provided that
the documentation makes this clear.
________
	case 'y':   /*  year within the century 00-99       */
	    sprintf(ptr, "%02d", tm->tm_year % 100);
	    break;
Should be (tm->tm_year + 1900) % 100, to handle years before 1900.
(Years BCE are not handled properly anyway by lots of things, and it's
too much bother to try).
________
	case 'Z':   /*  the name of the time zone or ""     */
	    strcpy(ptr, "");
	    break;
What does this do?  If you've decided to do nothing because you don't
know how to find the correct timezone (and I don't know how either),
then why not 
	case 'Z': /* I don't know how to do this */
		break;
Why the useless strcpy()?
This should be documented, so that individual users can change the
code to put in their local timezone.  Our implementation of strftime()
here simply puts in "EST", and this is clearly documented.
________
    i += strlen(buf + i);
What does this do?
_______
    return((size_t)i);
i should have been defined as a size_t in the first place.  If you
have already made the (quite reasonable) assumption that the size of
the buffer will fit comfortably in an int, why cast it now?
--
				Zev Sero  -  zvs@bby.oz.au
Violence is not a pleasant thing. It has caused much suffering in the world
since its invention, and many are convinced that it is Quite A Bad Thing.
					- Steven Megachiropter Foster