ICU API proposal: Change fIsTimeSet fAreFieldsSet fAreAllFieldsSet fAreFieldsVirtuallySet fIsSet fStamp to private

[Frank Tang (譚永鋒)]

Dear ICU team & users,

I would like to propose the following API for: ICU 77

Please provide feedback by: next Wednesday, 2024-12-18

Designated API reviewer: Markus

Ticket: https://unicode-org.atlassian.net/browse/ICU-22991 

I would like to propose a breaking change for the Calendar class in C++ which will only impact the subclass implementation of Calendar. After carefully studying the memory usage of the Calendar class, I found it very wasteful and there are many opportunities to improve it, but it will require some changes to the Calendar class which may break prior subclass implementation (is there any?)

The first step is to move some protected member data to private and disallow the subclass to access them directly. The alternative function is either unnecessary or already exist as protected (or public) methods

Here is the proposal, please look at 
https://docs.google.com/document/d/1Lyk4Dfpf5qGIbcmdjONkNjvgEqsXrD84I_X--9HQHtE/edit?tab=t.0

TLDR

Change fIsTimeSet fAreFieldsSet fAreAllFieldsSet fAreFieldsVirtuallySet fIsSet fStamp to private

I would like to propose the following changes to C++ Calendar class for ICU 77:

In calendar.h Change fIsTimeSet, fAreFieldsSet, fAreAllFieldsSet, fAreFieldsVirtuallySet, fIsSet, fStamp from protected to private.

These fields are not needed for direct access from subclass. Change them from

protected to private empower us to optimize the memory usage in follow through PR.

in i18n/unicode/calendar.h

- protected:

+ private:

    /**

     * The flag which indicates if the current time is set in the calendar.

-    * @stable ICU 2.0

     */

    UBool      fIsTimeSet;

    /**

     * True if the fields are in sync with the currently set time of this Calendar.

     * If false, then the next attempt to get the value of a field will

     * force a recomputation of all fields from the current value of the time

     * field.

     * <P>

     * This should really be named areFieldsInSync, but the old name is retained

     * for backward compatibility.

-    * @stable ICU 2.0

     */

    UBool      fAreFieldsSet;

    /**

     * True if all of the fields have been set.  This is initially false, and set to

     * true by computeFields().

-    * @stable ICU 2.0

     */

    UBool      fAreAllFieldsSet;

    /**

     * True if all fields have been virtually set, but have not yet been

     * computed.  This occurs only in setTimeInMillis().  A calendar set

     * to this state will compute all fields from the time if it becomes

     * necessary, but otherwise will delay such computation.

-    * @stable ICU 3.0

     */

    UBool fAreFieldsVirtuallySet;

#ifndef U_FORCE_HIDE_DEPRECATED_API

    /**

     * The flags which tell if a specified time field for the calendar is set.

-    * @deprecated ICU 2.8 use (fStamp[n]!=kUnset)

     */

    UBool      fIsSet[UCAL_FIELD_COUNT];

#endif  // U_FORCE_HIDE_DEPRECATED_API

    /**

     * Pseudo-time-stamps which specify when each field was set. There

     * are two special values, UNSET and INTERNALLY_SET. Values from

     * MINIMUM_USER_SET to Integer.MAX_VALUE are legal user set values.

-    * @stable ICU 2.0

     */

    int32_t        fStamp[UCAL_FIELD_COUNT];

--

Frank Yung-Fong Tang
譚永鋒 / 🌭🍊 
Sr. Software Engineer 

[Frank Tang (譚永鋒)]

I want to point out, currently, there are no unit test inside test/intltest to test and guarantee the correctness of these protected fields. In other words, while these member data were declared protected and accessible by subclass, there are currently no mechanism in our project to assure the accuracy of the values stored inside them. In other words, currently, we only allow these fields to be read/write by the subclass of Calendar, without assuring any quality of the value stored inside them. Therefore, we can easily break the subclass by changing how the values are stored in them without even changing them from protected to private.

Also, most of them are either needed for just the Calendar class itself (not the subclass), or could be replaced by calling to one of the following three public/protected functions: isSet()/ newestStamp() / newerField() 

For example https://github.com/unicode-org/icu/pull/3302

https://unicode-org.github.io/icu-docs/apidoc/dev/icu4c/classicu_1_1Calendar.html#a2965160eb8f038aab15467c05e257777

https://unicode-org.github.io/icu-docs/apidoc/dev/icu4c/classicu_1_1Calendar.html#a0a2b6e85ae40f89999d3170c291f1d26

https://unicode-org.github.io/icu-docs/apidoc/dev/icu4c/classicu_1_1Calendar.html#aa53b73c4da0321e4567e8189b58c919f

Regards,

Frank 

[Markus Scherer]

Makes sense to me.

We should document the functions that one should call if they had been accessing the fields directly. Maybe in the ticket, since we point there from both the pull request and from the release notes.

tnx

markus

[Rich Gillam]

Frank--

...Also, most of them are either needed for just the Calendar class itself (not the subclass), or could be replaced by calling to one of the following three public/protected functions…

“Most”?

Are you aware of any counterexamples?

—Rich

[Frank Tang (譚永鋒)]

fIsTimeSet fAreFieldsSet fAreAllFieldsSet fAreFieldsVirtuallySet fIsSet fStamp

Most mean the first 4 - fIsTimeSet fAreFieldsSet fAreAllFieldsSet fAreFieldsVirtuallySet

Is it possible you can search your internal code to see any code inside your company access the following?

fIsTimeSet fAreFieldsSet fAreAllFieldsSet fAreFieldsVirtuallySet fIsSet fStamp

[Dragan Bešević]

I double checked our code and we don't call any of those properties directly in Apple. So, Apple should be OK with this change.

--
You received this message because you are subscribed to the Google Groups "icu-design" group.
To unsubscribe from this group and stop receiving emails from it, send an email to icu-design+...@unicode.org.
To view this discussion visit https://groups.google.com/a/unicode.org/d/msgid/icu-design/CA%2B7fzPEAiEVZtkHW9%3DyDGWsG1uUZhkMCKbpkxA8ZUnkVoTG1Ag%40mail.gmail.com.
For more options, visit https://groups.google.com/a/unicode.org/d/optout.

--
You received this message because you are subscribed to the Google Groups "ICU - Team" group.
To unsubscribe from this group and stop receiving emails from it, send an email to icu-team+u...@unicode.org.
To view this discussion visit https://groups.google.com/a/unicode.org/d/msgid/icu-team/CA%2B7fzPEAiEVZtkHW9%3DyDGWsG1uUZhkMCKbpkxA8ZUnkVoTG1Ag%40mail.gmail.com.

[Frank Tang (譚永鋒)]

Do we know any other companies that may have a higher probability to add code to implement subclass of ICU Calendar? Any guesses?

[Markus Scherer]

On Tue, Dec 17, 2024 at 9:30 AM 'Frank Tang (譚永鋒)' via icu-design <icu-d...@unicode.org> wrote:

Do we know any other companies that may have a higher probability to add code to implement subclass of ICU Calendar? Any guesses?

IBM is no longer involved.

I assume you checked Google/Android/Chromium code?

markus

[Frank Tang (譚永鋒)]

Correct.

 

markus

[Frank Tang (譚永鋒)]

Also I want to point out while these variables were declared protected, 

1. The definition of how to use them is very vaguely written in the API doc. I wonder if anyone who implements the subclass will be able to figure out how to read them to do the right thing. 

fIsTimeSet fAreFieldsSet fAreAllFieldsSet fAreFieldsVirtuallySet are really state variable that the Calendar (not the subclass) maintain for the calculation of each field

fIsSet array is duplicated information now from fStamp

The one which is really needed and helpful for subclasses is fStamp , but the right way to use that is through two protected methods instead of directly reading it.

2. There are no unit tests to ensure or demonstrate how the values stored inside are nor what should be the correct/incorrect values. Therefore, these fields are right now really just an accessible memory without any guarantee to follow any API definition beyond you can read/write to them from subclass.