Hi Jason,
First of all, I think the goal of #7524 is to provide the functionalities
which are not accessible from C APIs. I think these are:
- detecting transition time (backward/forward) - BasicTimeZone
- creating a time zone from rule text in VTIMEZONE format - VTimeZone
TimeZone itself has two main features
- create a time zone for a given ID
- get UTC offset of a zone at a given time
I'd expect these features are covered in this proposal. But it looks some
important aspects (BasicTimeZone/VTimeZone) are missing while some much
less important things are included.
It looks your approach is trying to provide simple C API wrapper for C++
TimeZone class. I personally do not think we need to expose every C++
TimeZone APIs in C.
For now, I disagree with the proposal. We need to brush up APIs - correct
errors, remove unnecessary APIs and add necessary APIs.
We should also decide whether we really want to use utimezone_ prefix or
use ucal_ prefix instead.
I added comments below.
Jason J Spieth <spieth@...> wrote on 06/09/2010 04:17:02 PM:
>
> Dear ICU team & users,
>
> Originally I have a user request to create a wrapper for the
> VTimeZone class. However it was determined that such a wrapper for
> this specific time zone subclass would be rather restrictive. A
> more useful solution is to create a wrapper for the base time zone
> class. The wrapper for the base class will expose some basic
> functions. However, Some APIs in the current TimeZone class were
> designed when we did not support historic rules, functions such as
> getRawOffset, getDSTSavings and some others. These are not included
> in the wrapper at this time. Also the DisplayName APIs will not be
> wrapped as they have similar restrictions in that a time zone name
> can change over time. These out of date interfaces are not wrapped.
>
> For details see http://www.icu-project.org/trac/ticket/7524
>
> Please provide feedback by Wed. June 16.
> I would like to designate Yoshito as reviewer.
>
> Here is the specification for the wrapper:
>
>
> #ifndef UCNV_H
> struct UTimeZone;
> /**
> * Use the utimezone_* API to manipulate. Create with
> * utimezone_open*, and destroy with utimezone_close.
There is no "utimezone_open*" in your proposal.
> * @draft ICU 4.6
> */
> typedef struct UTimeZone UTimeZone;
> #endif
>
> /**
> * Creates a TimeZone for the given ID.
> * @param ID: the ID for a TimeZone, such as "America/Los_Angeles",
> * or a custom ID such as "GMT-8:00".
> * @param IDLength: the length of the ID
> * @return the specified TimeZone, or the GMT zone if the given ID
> * cannot be understood. Return result guaranteed to be non-null. If
you
> * require that the specific zone asked for be returned, check the ID of
the
> * return result.
> * @draft ICU 4.6
> */
> U_DRAFT UTimeZone* U_EXPORT2
> utimezone_createTimeZone(const UChar* ID,
> int32_t* IDLength);
I always feel C++ TimeZone::createTimeZone is problematic. It silently
fallback to GMT if the given ID is unknown. I do not think we need to
duplicate the bad API from C++ TimeZone here. In fact, we should probably
add another createTimeZone in C++ which returns status, instead of
silently fallback to GMT.
>
>
> /**
> * Creates a new copy of the default TimeZone for this host. Unless
> the default time
> * zone has already been set using adoptDefault() or setDefault(),
> the default is
> * determined by querying the system using methods in
> TPlatformUtilities. If the
> * system routines fail, or if they specify a TimeZone or TimeZone
> offset which is not
> * recognized, the TimeZone indicated by the ID kLastResortID is
instantiated
> * and made the default.
What is kLastResourceID? I know you copied API doc from C++ API. But I
think it's an error. We do not expose anything like kLastResourceID if my
understanding is correct.
> *
> * @return A default TimeZone. Clients are responsible for
> deleting the time zone
> * object returned.
> * @draft ICU 4.6
> */
> U_DRAFT UTimeZone* U_EXPORT2
> utimezone_createDefault(void);
I'm not sure you need to "create" a new instance. If we exclude any
"setters", this API can return "const UTimeZone*".
>
>
> /**
> * Disposes of the storage used by a UTimeZone object. This function
should
> * be called exactly once for objects returned by utimezone_open*.
> * @param zone: the object to dispose of
> * @draft ICU 4.6
> */
> U_DRAFT void U_EXPORT2
> utimezone_close(UTimeZone* zone);
>
>
> /**
> * Clones TimeZone objects polymorphically. Clients are responsible
> for deleting
> * the TimeZone object cloned.
> * @param zone: the time zone to clone
> * @return A new copy of this TimeZone object.
> * @draft ICU 4.6
> */
> U_DRAFT UTimeZone* U_EXPORT2
> utimezone_clone(UTimeZone* zone);
>
I do not think we need this function. In which case you actually want to
create a copy?
>
> /**
> * The GMT time zone has a raw offset of zero and does not use daylight
> * savings time. This is a commonly used time zone.
> * @param: zone: the time zone to use
> * @return the GMT time zone.
> * @draft ICU 4.6
> */
> U_DRAFT UTimeZone* U_EXPORT2
> utimezone_getGMT(UTimeZone* zone);
>
I do not understand how the input arg "UTimeZone* zone" is used.
I think this API should be "const UTimeZone* utimezone_getGMT(void)"
>
> /**
> * Returns the number of IDs in the equivalency group that
> * includes the given ID. An equivalency group contains zones
> * that have the same GMT offset and rules.
> *
> * <p>The returned count includes the given ID; it is always >= 1.
> * The given ID must be a system time zone. If it is not, returns
> * zero.
> *
> * @param: zone: the time zone to use
> * @param id: a system time zone ID
> * @param idLength: length of the id
> * @return the number of zones in the equivalency group containing
> * 'id', or zero if 'id' is not a valid system ID
> * @see #getEquivalentID
> * @draft ICU 4.6
> */
> U_DRAFT int32_t U_EXPORT2
> utimezone_countEquivalentIDs(UTimeZone* zone,
> const UChar* ID,
> int32_t* IDLength);
>
>
> /**
> * Returns an ID in the equivalency group that
> * includes the given ID. An equivalency group contains zones
> * that have the same GMT offset and rules.
> *
> * <p>The given index must be in the range 0..n-1, where n is the
> * value returned by <code>countEquivalentIDs(id)</code>. For
> * some value of 'index', the returned value will be equal to the
> * given id. If the given id is not a valid system time zone, or
> * if 'index' is out of range, then returns an empty string.
> *
> * @param: zone: the time zone to use
> * @param id: a system time zone ID
> * @param idLength: the length of the id
> * @param index a value from 0 to n-1, where n is the value
> * returned by <code>countEquivalentIDs(id)</code>
> * @param: equivId: the returned id of the index-th zone in the
> * equivalency group containing 'id', or an empty string if 'id' is
> not a valid
> * system ID or 'index' is out of range
> * @param: equivId: the length of the retuned equiv id. If equivId is
null,
> * this will contain the number of bytes to contain the equivId.
> *
> * @see #countEquivalentIDs
> * @draft ICU 4.6
> */
> U_DRAFT void U_EXPORT2
> utimezone_getEquivalentID(UTimeZone* zone,
> const UChar* id,
> int32_t* idLength,
> int32_t index,
> UChar* equivId,
> int32_t* equivIdLength);
>
>
The semantics of "equivalent" is somewhat ambiguous. In the existing C++
implementation, "equivalent" means that one time zone is a "Link" of
another time zone in the tz database. But, we also have another
"equivalent" derrived from CLDR alias mapping. Unfortunately, they are
not guranteed to match. I think "Link" in the tz database just specify
that one zone use the same set of rules with another. Technically, you
can specify these two time zones without using "Link" statement. In this
case, ICU TimeZone implementation assume they are not equivalent. Thus,
even now, C++ implementation has some potential problems.
I do not think this API is really important for most of ICU users. Of
course, we cannot remove the APIs in TimeZone C++ implementation. But, I
do not think it is worth adding this to C API set. So I propose to remove
these from the proposal.
> /**
> * Sets the default time zone (i.e., what's returned by
> createDefault()) to be the
> * specified time zone. If NULL is specified for the time zone, the
> default time
> * zone is set to the default host time zone. This call adopts the
> TimeZone object
> * passed in; the clent is no longer responsible for deleting it.
> *
> * @param zone: the time zone to use
> * @param dftZone: A pointer to the new TimeZone object to use as
> the default.
> * @draft ICU 4.6
> */
> U_DRAFT void U_EXPORT2
> utimezone_adoptDefault(UTimeZone* zone,
> UTimeZone* dftZone);
>
>
>
> /**
> * Same as adoptDefault(), except that the TimeZone object passed in
> is NOT adopted;
> * the caller remains responsible for deleting it.
> *
> * @param zone: the time zone to use
> * @param dftZone: a pointer to the TimeZone object to use as the
default.
> * @system
> * @draft ICU 4.6
> */
> U_DRAFT void U_EXPORT2
> utimezone_setDefault(UTimeZone* zone,
> const UTimeZone* dftZone);
>
>
First of all, these API signatures are wrong. These APIs simply take one
"UTimeZone*" argument.
Also, I feel these APIs are not critical. I do not see practical use case
to "set" default time zone in regular applications. Even we want such,
one is good enough.
> /**
> * Returns the timezone data version currently used by ICU.
> * @param zone: the time zone to use
> * @param status: Output param to filled in with a success or an error.
> * @return the data version
> * @draft ICU 4.6
> */
> U_DRAFT const char* U_EXPORT2
> utimezone_getTZDataVersion(UTimeZone* zone,
> UErrorCode* status);
>
>
We alredy have ucal_getTZDataVersion. This API also does not make sense -
how do you use the first argument "UTimeZone* zone"?
> /**
> * Returns the canonical system timezone ID or the normalized
> * custom time zone ID for the given time zone ID.
> * @param zone: the time zone to use
> * @param id The input time zone ID to be canonicalized.
> * @param idLength: The length of the id
> * @param canonicalID Receives the canonical system time zone ID
> * or the custom time zone ID in normalized format.
> * @param canonicalIDLen: Length of the canonicalID. If the canonicalID
is
> * null this will contain the number of bytes
needed
> * to hold the canonicalID
> * @param status Recevies the status. When the given time zone
ID
> * is neither a known system time zone ID nor a
> * valid custom time zone ID,
U_ILLEGAL_ARGUMENT_ERROR
> * is set.
> * @draft ICU 4.6
> */
> U_DRAFT void U_EXPORT2
> utimezone_getCanonicalID(UTimeZone* zone,
> const UChar* id,
> int32_t* idLength,
> UChar* canonicalID,
> int32_t* canonicalIDLen,
> UErrorCode* status);
>
>
> /**
> * Returns the canonical system time zone ID or the normalized
> * custom time zone ID for the given time zone ID.
> * @param zone: the time zone to use
> * @param id The input time zone ID to be canonicalized.
> * @param idLength: The length of the id
> * @param canonicalID Receives the canonical system time zone ID
> * or the custom time zone ID in normalized format.
> * @param canonicalIDLen: Length of the canonicalID. If the canonicalID
is
> * null this will contain the number of bytes
needed
> * to hold the canonicalID
> * @param isSystemID Receives if the given ID is a known system
> * time zone ID.
> * @param status Recevies the status. When the given time zone
ID
> * is neither a known system time zone ID nor a
> * valid custom time zone ID,
U_ILLEGAL_ARGUMENT_ERROR
> * is set.
> * @draft ICU 4.6
> */
> U_DRAFT void U_EXPORT2
> utimezone_getCanonicalIDSys(UTimeZone* zone,
> const UChar* id,
> int32_t* idLength,
> UChar* canonicalID,
> int32_t* canonicalIDLen,
> UBool* isSystemID,
> UErrorCode* status);
>
We already have ucal_getCanonicalTimeZoneID. Same question - how do you
use the first arg "UTimeZone*" in these functions?
>
> /**
> * Returns true if the two TimeZones are equal. (The TimeZone
> version only compares
> * IDs, but subclasses are expected to also compare the fields they
add.)
subclasses?
> *
> * @param zone1: the time zone to be checked for equality
> * @param zone2: the time zone to be checked for equality
> * @return true if the test condition is met
> * @draft ICU 4.6
> */
> U_DRAFT UBool U_EXPORT2
> utimezone_equals(const UTimeZone* zone1,
> const UTimeZone* zone2);
>
I do not see much need for this API. I would remove this from the
proposal.
>
> /**
> * Returns the time zone raw and GMT offset for the given moment
> * in time. Upon return, local-millis = GMT-millis + rawOffset +
> * dstOffset. All computations are performed in the proleptic
> * Gregorian calendar. The default implementation in the TimeZone
> * class delegates to the 8-argument getOffset().
> *
> * @param: zone: the time zone to use
> * @param date moment in time for which to return offsets, in
> * units of milliseconds from January 1, 1970 0:00 GMT, either GMT
> * time or local wall time, depending on `local'.
> * @param local if true, `date' is local wall time; otherwise it
> * is in GMT time.
> * @param rawOffset output parameter to receive the raw offset, that
> * is, the offset not including DST adjustments
> * @param dstOffset output parameter to receive the DST offset,
> * that is, the offset to be added to `rawOffset' to obtain the
> * total offset between local and GMT time. If DST is not in
> * effect, this value is zero; otherwise it is a positive value,
> * typically one hour.
> * @param ec input-output error code
> *
> * @draft ICU 4.6
> */
> U_DRAFT void U_EXPORT2
> utimezone_getOffset(UTimeZone* zone,
> UDate date,
> UBool local,
> int32_t* rawOffset,
> int32_t* dstOffset,
> UErrorCode* ec);
>
> /**
> * Fills in "ID" with the TimeZone's ID.
> *
> * @param zone: the time zone to use
> * @param ID Receives this TimeZone's ID.
> * @param IDLength: the length of ID. If ID is null
> * this will return the number of bytes needed for ID
> * @draft ICU 4.6
> */
> U_DRAFT void U_EXPORT2
> utimezone_getID(UTimeZone* zone,
> UChar* ID,
> int32_t* IDLength);
>
>
> /**
> * Sets the TimeZone's ID to the specified value. This doesn't
> affect any other
> * fields.
> *
> * @param zone: the time zone to use
> * @param ID The new time zone ID.
> * @param IDLength: the length of ID.
> * @draft ICU 4.6
> */
> U_DRAFT void U_EXPORT2
> utimezone_setID(UTimeZone* zone,
> UChar* ID,
> int32_t* IDLength);
>
I do not think we need utimezone_setID. I strongly feel that time zone
should not have any state.
For getID - I'm not sure. I think it's not important. I would remove
both of above from the proposal. But someone else thinks it's important,
I can live with it.
> /**
> * Returns true if this zone has the same rule and offset as another
zone.
> * That is, if this zone differs only in ID, if at all.
> * @param zone: the time zone to use
> * @param other the <code>TimeZone</code> object to be compared with
Above description should be updated for C API.
> * @return true if the given zone is the same as this one,
> * with the possible exception of the ID
> * @draft ICU 4.6
> */
> U_DRAFT UBool U_EXPORT2
> utimezone_hasSameRules(UTimeZone* zone,
> UTimeZone* other);
I think this is probably more useful than getEquivalentID. But I do not
think any critical use case. I would remove this for this proposal until
someone really requests this.
-Yoshito
|