Data Driven Tests for DecimalFormat.

Data Driven Tests for DecimalFormat.

[andy.heninger, 2015-05-26 23:30:27]

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/Makefile.in
File source/test/intltest/Makefile.in (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/Makefile...
source/test/intltest/Makefile.in:61: scientificnumberformattertest.o
datadrivennumberformattestsuite.o \
We are going to need the corresponding additions to the visual studio project
files.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/datadriv...
File source/test/intltest/datadrivennumberformattestsuite.h (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/datadriv...
source/test/intltest/datadrivennumberformattestsuite.h:18: U_NAMESPACE_BEGIN
Tests shouldn't be in the icu name space.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/datadriv...
source/test/intltest/datadrivennumberformattestsuite.h:23: class
DataDrivenNumberFormatTestSuite : public UObject {
Tests aren't ICU library classes, so no UObject. Tests almost always inherit
from class IntlTest.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/intltest.h
File source/test/intltest/intltest.h (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/intltest...
source/test/intltest/intltest.h:331: friend class
DataDrivenNumberFormatTestSuite;
Why does this need to be here? Seems really out of place, no other test needs
it, why this one.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
File source/test/intltest/numberformattesttuple.cpp (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:7: 
Please add some brief description saying what this class is generally for, how
it fits into the scheme of things.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:15: U_NAMESPACE_BEGIN
Test classes don't go in the ICU name space.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:17: NumberFormatTestTuple
*gNullPtr = NULL;
Make this static. gNullPtr will be exported.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:19: #define
FIELD_OFFSET(fieldName) ((int32_t) (((char *) &gNullPtr->fieldName) - ((char *)
gNullPtr)))
This kind of pointer arithmetic and casting makes me nervous, it seems like the
various static checker tools keep ratcheting up the things they complain about.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numfmtst...
File source/test/intltest/numfmtst.cpp (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numfmtst...
source/test/intltest/numfmtst.cpp:38: class NumberFormatTestDataDriven : public
icu::DataDrivenNumberFormatTestSuite {
Style-wise, I would prefer the list of test cases remain very near the top of
this file, as is the case with nearly all ICU test source files.

Why do we need a class hierarchy for only one derived class? Could we move all
of this into DataDrivenNumberFormatTestSuite?

https://codereview.appspot.com/239300043/diff/1/source/test/testdata/numberfo...
File source/test/testdata/numberformattestspecification.txt (right):

https://codereview.appspot.com/239300043/diff/1/source/test/testdata/numberfo...
source/test/testdata/numberformattestspecification.txt:1: test basic patterns
It would be very helpful to have a description of the syntax and available
options at the top of this file. For the benefit of people who will be adding
new tests cases.

[tkeep, 2015-05-27 18:02:43]

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/Makefile.in
File source/test/intltest/Makefile.in (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/Makefile...
source/test/intltest/Makefile.in:61: scientificnumberformattertest.o
datadrivennumberformattestsuite.o \
On 2015/05/26 23:30:26, andy.heninger wrote:
> We are going to need the corresponding additions to the visual studio project
> files.

Will do this part in a separate commit.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/datadriv...
File source/test/intltest/datadrivennumberformattestsuite.h (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/datadriv...
source/test/intltest/datadrivennumberformattestsuite.h:18: U_NAMESPACE_BEGIN
On 2015/05/26 23:30:26, andy.heninger wrote:
> Tests shouldn't be in the icu name space.

Done.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/datadriv...
source/test/intltest/datadrivennumberformattestsuite.h:23: class
DataDrivenNumberFormatTestSuite : public UObject {
On 2015/05/26 23:30:26, andy.heninger wrote:
> Tests aren't ICU library classes, so no UObject. Tests almost always inherit
> from class IntlTest.

Done.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/intltest.h
File source/test/intltest/intltest.h (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/intltest...
source/test/intltest/intltest.h:331: friend class
DataDrivenNumberFormatTestSuite;
On 2015/05/26 23:30:26, andy.heninger wrote:
> Why does this need to be here? Seems really out of place, no other test needs
> it, why this one.

Done.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
File source/test/intltest/numberformattesttuple.cpp (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:7: 
On 2015/05/26 23:30:26, andy.heninger wrote:
> Please add some brief description saying what this class is generally for, how
> it fits into the scheme of things.

Documentation added to the header file.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:15: U_NAMESPACE_BEGIN
On 2015/05/26 23:30:26, andy.heninger wrote:
> Test classes don't go in the ICU name space.

Done.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:17: NumberFormatTestTuple
*gNullPtr = NULL;
On 2015/05/26 23:30:26, andy.heninger wrote:
> Make this static. gNullPtr will be exported.

Done.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:19: #define
FIELD_OFFSET(fieldName) ((int32_t) (((char *) &gNullPtr->fieldName) - ((char *)
gNullPtr)))
On 2015/05/26 23:30:26, andy.heninger wrote:
> This kind of pointer arithmetic and casting makes me nervous, it seems like
the
> various static checker tools keep ratcheting up the things they complain
about.

How should we proceed then?

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
File source/test/intltest/numberformattesttuple.h (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.h:63: class NumberFormatTestTuple :
UMemory {
Andy - This class isn't a test, but it isn't library code either. Should it
subclass UMemory?

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numfmtst...
File source/test/intltest/numfmtst.cpp (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numfmtst...
source/test/intltest/numfmtst.cpp:38: class NumberFormatTestDataDriven : public
icu::DataDrivenNumberFormatTestSuite {
On 2015/05/26 23:30:26, andy.heninger wrote:
> Style-wise, I would prefer the list of test cases remain very near the top of
> this file, as is the case with nearly all ICU test source files.
> 
> Why do we need a class hierarchy for only one derived class? Could we move all
> of this into DataDrivenNumberFormatTestSuite?
> 

This is an abstract class because I had two implementations of this class, one
that tested DecimalFormat and one that tested DecimalFormat2. This polymorphism
is what allowed me to use the same test file to test both the legacy code and
the new code. The purpose of that exercise was to ensure that the new code was
correct. Now that the new code is gone, I understand why this now seems overly
complicated.

https://codereview.appspot.com/239300043/diff/1/source/test/testdata/numberfo...
File source/test/testdata/numberformattestspecification.txt (right):

https://codereview.appspot.com/239300043/diff/1/source/test/testdata/numberfo...
source/test/testdata/numberformattestspecification.txt:1: test basic patterns
On 2015/05/26 23:30:26, andy.heninger wrote:
> It would be very helpful to have a description of the syntax and available
> options at the top of this file. For the benefit of people who will be adding
> new tests cases.

Done.

[andy.heninger, 2015-05-27 21:33:24]

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
File source/test/intltest/numberformattesttuple.cpp (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:19: #define
FIELD_OFFSET(fieldName) ((int32_t) (((char *) &gNullPtr->fieldName) - ((char *)
gNullPtr)))
On 2015/05/26 23:30:26, andy.heninger wrote:
> This kind of pointer arithmetic and casting makes me nervous, it seems like
the
> various static checker tools keep ratcheting up the things they complain
about.

Maybe there is some way to use a C++ pointer to data member, which is pretty
much the same thing inside, an offset within an object.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:287: const
NumberFormatTestTupleFieldOps *ops;
So we are assembling structs with virtual functions implemented as pointers to C
functions.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:441: 
Here's an approach to doing the tuple field access without all the casting and
unsafe pointer arithmetic, but otherwise keeping the structure similar to what
it is.

class Tuple {
  public:
    int32_t       a;
    int32_t       b;
    int32_t       c;

    UnicodeString s;
    double        d;

    void setByName(const char *field, UnicodeString value);
    UnicodeString &appendByName(const char *field, UnicodeString &appendTo);
};

struct TupleItem {
    TupleItem(const char *name) {fName = name;};

    const char *fName;
    virtual void set(Tuple *tuple, const UnicodeString &value) = 0;
    virtual UnicodeString &toString(Tuple *tuple, UnicodeString &appendTo) = 0;
};   

struct IntItem: public TupleItem {
    IntItem(const char *name, int Tuple::*pdata) : TupleItem(name),
fpdata(pdata) {}; 
    int32_t Tuple::*fpdata;

    void set(Tuple *tuple, const UnicodeString &value) {
        // value = convert the string.
        tuple->*fpdata = 0  /* value */;
    }

    UnicodeString &toString(Tuple *tuple, UnicodeString &appendTo) {
        int32_t v = tuple->*fpdata;
        // Append v to the string.
        return appendTo;
    }
};

struct StringItem: public TupleItem {
    StringItem(const char *name, UnicodeString Tuple::*pdata) : TupleItem(name),
fpdata(pdata) {}; 
    UnicodeString Tuple::*fpdata;

    void set(Tuple *tuple, const UnicodeString &value) {
        tuple->*fpdata = value;
    }
    
    UnicodeString &toString(Tuple *tuple, UnicodeString &appendTo) {
        appendTo.append(tuple->*fpdata);
        return appendTo;
    }
};


int main() {
    TupleItem *tupleDesc[] = {
        new IntItem("a", &Tuple::a),
        new IntItem("b", &Tuple::b),
        new IntItem("c", &Tuple::c),
        new StringItem("s", &Tuple::s)
    };

Not clear if its worth changing, though, since its working as it is.

The thing is really a map of field names to fields. Too bad we can't use std
classes. UHash might save a few lines of code in handling name lookups, and keep
the intent clear.

[tkeep, 2015-05-27 21:57:12]

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/Makefile.in
File source/test/intltest/Makefile.in (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/Makefile...
source/test/intltest/Makefile.in:61: scientificnumberformattertest.o
datadrivennumberformattestsuite.o \
On 2015/05/27 18:02:43, tkeep wrote:
> On 2015/05/26 23:30:26, andy.heninger wrote:
> > We are going to need the corresponding additions to the visual studio
project
> > files.
> 
> Will do this part in a separate commit.

Done.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
File source/test/intltest/numberformattesttuple.cpp (right):

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:287: const
NumberFormatTestTupleFieldOps *ops;
On 2015/05/27 21:33:24, andy.heninger wrote:
> So we are assembling structs with virtual functions implemented as pointers to
C
> functions.

Not sure what you are asking me to do here.

https://codereview.appspot.com/239300043/diff/1/source/test/intltest/numberfo...
source/test/intltest/numberformattesttuple.cpp:441: 
On 2015/05/27 21:33:24, andy.heninger wrote:
> Here's an approach to doing the tuple field access without all the casting and
> unsafe pointer arithmetic, but otherwise keeping the structure similar to what
> it is.
> 
> class Tuple {
>   public:
>     int32_t       a;
>     int32_t       b;
>     int32_t       c;
> 
>     UnicodeString s;
>     double        d;
> 
>     void setByName(const char *field, UnicodeString value);
>     UnicodeString &appendByName(const char *field, UnicodeString &appendTo);
> };
> 
> struct TupleItem {
>     TupleItem(const char *name) {fName = name;};
> 
>     const char *fName;
>     virtual void set(Tuple *tuple, const UnicodeString &value) = 0;
>     virtual UnicodeString &toString(Tuple *tuple, UnicodeString &appendTo) =
0;
> };   
> 
> struct IntItem: public TupleItem {
>     IntItem(const char *name, int Tuple::*pdata) : TupleItem(name),
> fpdata(pdata) {}; 
>     int32_t Tuple::*fpdata;
> 
>     void set(Tuple *tuple, const UnicodeString &value) {
>         // value = convert the string.
>         tuple->*fpdata = 0  /* value */;
>     }
> 
>     UnicodeString &toString(Tuple *tuple, UnicodeString &appendTo) {
>         int32_t v = tuple->*fpdata;
>         // Append v to the string.
>         return appendTo;
>     }
> };
> 
> struct StringItem: public TupleItem {
>     StringItem(const char *name, UnicodeString Tuple::*pdata) :
TupleItem(name),
> fpdata(pdata) {}; 
>     UnicodeString Tuple::*fpdata;
> 
>     void set(Tuple *tuple, const UnicodeString &value) {
>         tuple->*fpdata = value;
>     }
>     
>     UnicodeString &toString(Tuple *tuple, UnicodeString &appendTo) {
>         appendTo.append(tuple->*fpdata);
>         return appendTo;
>     }
> };
> 
> 
> int main() {
>     TupleItem *tupleDesc[] = {
>         new IntItem("a", &Tuple::a),
>         new IntItem("b", &Tuple::b),
>         new IntItem("c", &Tuple::c),
>         new StringItem("s", &Tuple::s)
>     };
> 
> Not clear if its worth changing, though, since its working as it is.
> 
> The thing is really a map of field names to fields. Too bad we can't use std
> classes. UHash might save a few lines of code in handling name lookups, and
keep
> the intent clear.

Are you ok with trying what I have out in the trunk? If it breaks, we will know
within 1 to 2 hours. Then we can decide whether doing this change is worth it?
Does that sound reasonable? Of course I have never rolled back a commit to trunk
before.