ticket:11985: Handle 'j' and 'J' in date time pattern skeletons.

ticket:11985: Handle 'j' and 'J' in date time pattern skeletons.

R=mark.edward.davis@gmail.com, markus.icu@gmail.com, pedberg@apple.com

Committed: http://bugs.icu-project.org/trac/changeset/38089

[markus.icu, 2015-11-13 00:45:50]

Looks plausible to me.

Please also add test cases with j/J just calling staticGetSkeleton().

https://codereview.appspot.com/279780043/diff/20001/source/test/intltest/dtfm...
File source/test/intltest/dtfmttst.cpp (right):

https://codereview.appspot.com/279780043/diff/20001/source/test/intltest/dtfm...
source/test/intltest/dtfmttst.cpp:4841: void DateFormatTest::TestTicket11985() {
Optional: I prefer a test name that is more obvious, for example
-TestEnglishForSkeletonWithJ(). You could put the ticket number into a comment.

[roubert (google), 2015-11-17 21:34:03]

On 2015/11/13 00:45:50, markus.icu wrote:
> Please also add test cases with j/J just calling staticGetSkeleton().

Done.

[roubert (google), 2015-11-17 21:34:17]

https://codereview.appspot.com/279780043/diff/20001/source/test/intltest/dtfm...
File source/test/intltest/dtfmttst.cpp (right):

https://codereview.appspot.com/279780043/diff/20001/source/test/intltest/dtfm...
source/test/intltest/dtfmttst.cpp:4841: void DateFormatTest::TestTicket11985() {
On 2015/11/13 00:45:49, markus.icu wrote:
> Optional: I prefer a test name that is more obvious, for example
> -TestEnglishForSkeletonWithJ(). You could put the ticket number into a
comment.

Done.

https://codereview.appspot.com/279780043/diff/40001/source/test/intltest/dtfm...
File source/test/intltest/dtfmttst.cpp (right):

https://codereview.appspot.com/279780043/diff/40001/source/test/intltest/dtfm...
source/test/intltest/dtfmttst.cpp:4847: {Locale::getEnglish(), "jmm", "h:mm a"},
These test cases pass just like this before changeset 37860 and still pass
after, with this change set applied.

[mark.edward.davis, 2015-11-18 12:15:29]

LGTM

https://codereview.appspot.com/279780043/diff/40001/source/test/intltest/dtfm...
File source/test/intltest/dtfmttst.cpp (right):

https://codereview.appspot.com/279780043/diff/40001/source/test/intltest/dtfm...
source/test/intltest/dtfmttst.cpp:4848: {Locale::getEnglish(), "Jmm", "hh:mm"},
Make these jj and JJ to reduce dependency on changes in locale data.

[roubert (google), 2015-11-18 12:28:39]

https://codereview.appspot.com/279780043/diff/40001/source/test/intltest/dtfm...
File source/test/intltest/dtfmttst.cpp (right):

https://codereview.appspot.com/279780043/diff/40001/source/test/intltest/dtfm...
source/test/intltest/dtfmttst.cpp:4848: {Locale::getEnglish(), "Jmm", "hh:mm"},
On 2015/11/18 12:15:29, mark.edward.davis wrote:
> Make these jj and JJ to reduce dependency on changes in locale data.

Done.

[roubert (google), 2015-11-18 12:29:24]

Committed patchset #4 (id:60001) manually as 38089 (presubmit successful).

[markus.icu, 2015-11-18 17:15:46]

In the future, please instead of
    sizeof TESTDATA / sizeof *TESTDATA
use
    UPRV_LENGTHOF(TESTDATA)

In intltest/dtfmttst.cpp, initializing a *static* Locale *reference* in the
TESTDATA from a factory method seems fishy. Why would the object not get
cleaned up? Might happen to work because it's initialized only once, used
right away, and never called again. I would remove the reference ampersand.

markus

[roubert (google), 2015-11-19 14:07:23]

On 2015/11/18 17:15:46, markus.icu wrote:

> In the future, please instead of
>     sizeof TESTDATA / sizeof *TESTDATA
> use
>     UPRV_LENGTHOF(TESTDATA)

OK.

The reason I went with sizeof/sizeof was that this is what's most commonly used
in the rest of that file, more than both the locally defined ARRAY_SIZE macro
and UPRV_LENGTHOF. Do you think it would be a good idea to sweep the entire
codebase to remove all uses of sizeof/sizeof (incl. local macro definitions) to
prevent others from doing the same mistake in the future?

> In intltest/dtfmttst.cpp, initializing a *static* Locale *reference* in the
> TESTDATA from a factory method seems fishy.

It might seem fishy, but it isn't. A temporary created to hold a reference
initializer persists until the end of its reference's scope. (This is
well-defined behaviour.)

[markus.icu, 2015-11-19 16:45:04]

On Thu, Nov 19, 2015 at 6:07 AM, <roubert@google.com> wrote:

> On 2015/11/18 17:15:46, markus.icu wrote:
>
> In the future, please instead of
>>      sizeof TESTDATA / sizeof *TESTDATA
>> use
>>      UPRV_LENGTHOF(TESTDATA)
>>
>
> OK.
>
> The reason I went with sizeof/sizeof was that this is what's most
> commonly used in the rest of that file, more than both the locally
> defined ARRAY_SIZE macro and UPRV_LENGTHOF. Do you think it would be a
> good idea to sweep the entire codebase to remove all uses of
> sizeof/sizeof (incl. local macro definitions) to prevent others from
> doing the same mistake in the future?
>

Maybe. I tend to think that we can handle this kind of thing in code
reviews.

If you fix this one, you could fix the whole file.

If you want to fix all of ICU, then don't put it into this ticket, submit a
new one, and we can see what people think.


> In intltest/dtfmttst.cpp, initializing a *static* Locale *reference*
>>
> in the
>
>> TESTDATA from a factory method seems fishy.
>>
>
> It might seem fishy, but it isn't. A temporary created to hold a
> reference initializer persists until the end of its reference's scope.
> (This is well-defined behaviour.)
>

Yes, but it's a *static* reference variable, so the *reference* should
persist until the end of time, but the temporary it refers to goes poof at
the end of the function scope. It's theoretical as long as the test
function is only called once, but it makes me queasy.

markus

[roubert (google), 2015-11-19 16:57:49]

On 2015/11/19 16:45:04, markus.icu wrote:

> If you want to fix all of ICU, then don't put it into this ticket, submit a
> new one, and we can see what people think.

OK.

> Yes, but it's a *static* reference variable, so the *reference* should
> persist until the end of time, but the temporary it refers to goes poof at
> the end of the function scope.

No, the temporary should exist as long as the reference to it exists. That's
what's special about temporaries created to hold a reference initializer. Take a
look at this example code:


#include <stdio.h>

struct A {
  int value;
  static A create() { return A{42}; }
};

int get() {
  static const A& a = A::create();
  return a.value;
} 

int main(int argc, char *argv[]) {
  printf("%d\n", get());
  printf("%d\n", get());
  return 0;
} 


As far as I know, this is valid C++ and prints 42 twice.

[markus.icu, 2015-11-19 17:08:28]

On Thu, Nov 19, 2015 at 8:57 AM, <roubert@google.com> wrote:

> Yes, but it's a *static* reference variable, so the *reference* should
>> persist until the end of time, but the temporary it refers to goes
>>
> poof at
>
>> the end of the function scope.
>>
>
> No, the temporary should exist as long as the reference to it exists.
> That's what's special about temporaries created to hold a reference
> initializer.


Hm. I guess that's... useful.
I wish C++ was more predictable!

Take a look at this example code:
>
>
> #include <stdio.h>
>
> struct A {
>   int value;
>

I added a destructor here:

  ~A() { puts("destructor"); }


>   static A create() { return A{42}; }
> };
>
> int get() {
>   static const A& a = A::create();
>   return a.value;
> }
>
> int main(int argc, char *argv[]) {
>   printf("%d\n", get());
>   printf("%d\n", get());
>   return 0;
> }
>
>
> As far as I know, this is valid C++ and prints 42 twice.
>

Yes, and for me it printed
42
42
destructor

Very weird... but you are right (at least for g++).

markus