Date: Tue, 9 May 2017 14:44:33 +0000 From: Kees Dekker <Kees.Dekker@infor.com> Message-ID: <DM5PR02MB2809951912FF7B2DE1A24AD5E9EF0@DM5PR02MB2809.namprd02.prod.outlook.com> | 2. Adding a faster (and more efficient) algorithm for mktime(). | Instead of the binary search another approach is used, I think that kind of thing is fine for a system's implementation of mktime(), but I don't think it is best for the reference implementation. The big advantage of the binary search algorithm, is that it moves *all* assumptions about the rules about the conversions into localtime, so they exist only in one place, and need to be updated only in one place. For example, ... + lDays += 1; /* year 0 is a leap year */ Is it indeed? Is there even a year 0? For sure, in the current implementation, we extend the Gregorian calendar way back into pre-history, and assume that years all had 365.25 days, and days all had 86400 seconds, even when we know that neither was true ... we extend that assumption back before the earth was formed, when there was nothing rotating and revolving around the sun, back even before there was a sun to rotate around, as if that all makes some kind of sense - which it doesn't. While fixing this in a way that would have made contemporary sense is hard, if not impossible, we could at least do better with pre-Gregorian human era dates - we could have the timezone data include the date/time at which the Gregorian calendar came into use in a particular zone, and some indication of what algorithm preceded it, and when that one came into being, etc, and for times before we have any idea what times were called we could just adopt stardates ... If we do any of that, it would be nice to have to write the code just once, in localtime(), and not have to also invent its inverse in mktime(). | 3. Adding a possibility not to use putenv(). That is certainly a worthwhile goal, though I would prefer to solve it by introducing a variant of tzset() which takes a timezone spec (anything which can be a value of getenv("TZ") as an arg, and returns a void * (pointer to an internal tzlib data struct) (it could be given some other type name, as long as internal details of its representation are not available) and then a parallel set of functions to localtime, mktime, etc which take this additional pointer as an extra arg, and use that zone. This allows parallel use of many different zones, without needing to be constantly switching between them, with all the costs that involves. We could perhaps call the tzset() variant, tzalloc() the data type it returns (rather than void *) a timezone_t, and add tzfree() to go with it of course, and the parallel functions could be localtime_rz() ctime_rz() mktime_z() ... And just maybe, all of this is already implemented and available... If the reason for doing it the other way is to avoid needing to change some application's localtime() (etc) calls, that can easily be handled by macros... #define tzset() (_my_tz = tzalloc(getenv("TZ"))) #define localtime(t) (localtime_rz(_my_tz, (t), &_my_static_tm)) /* ... */ | 4. Solved some platform (e.g. HPUX) specific errors/warnings | (initializing + some fixed char buffers). Assuming that this is the reason for ... - static const char wday_name[][3] = { + static const char wday_name[][4] = { Then IMO that system is just broken, it has always been legal to provide a string where the non-null chars exactly fill the space provided, in which case the terminating \0 is simply omitted from the initialisation. If some system has broken that, it should be made to suffer, we should not be working around it. | I'm not sure whether the way how I provide this patch is the | recommended way. It is not me who has to deal with it, but the patch format itself looked fine to me. I would suggest re-formatting the code into the style of the existing reference implementation, even if it is not your (or yor system's) preferred coding (and naming) style, if you expect the patches to be adopted though. A couple of the changes perplex me though (minor issues, but ...) Why: -# ifdef USG_COMPAT +# if defined(USG_COMPAT) When I first saw that, I assumed perhaps some system had deleted support for the (very old now, and no longer really needed) #ifdef, in which case that change would make sense, but then just after there is ... +#ifdef _MSC_VER where a new #ifdef is added, so that leaves the reason for the earlier change unexplained? +/* a lot of stuff does not exist on Windows/Visual Studio */ +#define DEFAULT_FOR_UNIX (!_MSC_VER) + #ifndef HAVE_LINK -#define HAVE_LINK 1 +#define HAVE_LINK DEFAULT_FOR_UNIX This kind of change should just be handled by pre-defining the HAVE_XXX symbols (with 0 values), that's why the #ifndef is there. kre