Mailing-List: contact stdcxx-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: stdcxx-dev@incubator.apache.org
Received-SPF: neutral (asf.osuosl.org: local policy)
Message-ID: <442463F1.8050608@roguewave.com>
Date: Fri, 24 Mar 2006 14:26:09 -0700
From: Martin Sebor <sebor@roguewave.com>
Organization: Rogue Wave Software
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
 rv:1.7.12) Gecko/20050915
MIME-Version: 1.0
To: stdcxx-dev@incubator.apache.org
Subject: Re: test for lib.string.insert
References: <4422CCCE.1090009@moscow.vdiweb.com>
In-Reply-To: <4422CCCE.1090009@moscow.vdiweb.com>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Anton Pevtsov wrote:
> The attached file contains the updated test for lib.string.insert.
[...]
> typedef enum InsertTags {
> 
>     // insert (size_type pos, const charT* s)
>     i_ptr            =  1,   

I would suggest to use i_pos_xxx as the convention for those members
that take size_type as the first argument to denote a position. That
way we can omit mentioning the first argument in the enumerations
for the iterator equivalents of these functions which are common
in the other containers (vector, etc.) So here I would go with
i_pos_ptr.

>     // insert (size_type pos, const basic_string& str)
>     i_str            =  2,

i_pos_str.

>     // insert (size_type pos, const charT* s, size_type n)
>     i_num_ptr        =  3,

How about i_pos_ptr_count instead, to preserve the order of the
arguments in the name of the constant?

>     // insert (size_type pos1, basic_string& str, size_type pos2, size_type n)
>     i_num_str        =  4,

Hmmm, i_pos_substr? (Or i_pos_str_pos_count?)

>     // insert (size_type pos, size_type n, charT c)
>     i_num_char       =  5,

I would suggest i_pos_count_val. Count for consistency with the rest
of the names and val instead of char because this overload is common
to all containers so we will be able to reuse the same name when we
do vector and others.

>     // insert (iterator p, charT c)
>     i_iters_num_char =  6,

I'd go with i_val.

>     // insert (iterator p, size_type n, charT c)
>     i_iters_char =      7,

i_count_val.

>     // insert (iterator p, InputIterator first, InputIterator last)
>     i_iters_range     = 8

i_range for simplicity?

Once we have a nice general naming convention worked out we can
move these names to a common header and reuse it in other tests,
and perhaps also in the driver itself. Come to think of it, that
might be the way to simplify the terribly complex assert strings
and conditions. We could add a formatting directive taking the
name of the container (basic_string, vector, etc.), the names
of its template arguments, and the member function tag or id
and the values of the function arguments, and have it format
the whole thing. This will need some thought but I like it! :)

> 
> } ITags;
> 
> /**************************************************************************/
> 
> static const struct TestCase
> {
>     int  line;
> 
>     int  pos1;
>     int  pos2;
>     int  num;
>     int  cnt;

Do we need both, num and cnt? If not, would it make sense to have
just one member (called count)?

>     char ch;

Shouldn't this be int so that it could be set to -1 when it doesn't
apply?

> 
>     const char* str;
>     std::size_t str_len;
> 
>     const char* src;
>     std::size_t src_len;
> 
>     const char* res;
>     std::size_t res_len;
> 
>     const char* c_res;
>     std::size_t c_res_len;

I would prefer to have just one result per test case. More on this
below...

> 
>     int bthrow;
> 
> } test_cases [] = {
> 
> #undef TEST
> #define TEST(str, pos1, src, pos2, num, cnt, ch, res, c_res, bthrow)         \
>     { __LINE__, pos1, pos2, num, cnt, ch, str, sizeof str - 1, src,          \
>       sizeof src - 1, res, sizeof res - 1, c_res, sizeof c_res - 1, bthrow }
> 
>     //    +----------------------------------------- controlled sequence
>     //    |      +---------------------------------- insert() pos argument
>     //    |      |  +------------------------------- sequence to be inserted
>     //    |      |  |          +-------------------- insert() pos2 argument
>     //    |      |  |          |  +----------------- insert() num argument 
>     //    |      |  |          |  |  +-------------- insert() count argument 
>     //    |      |  |          |  |  |  +----------- character to be inserted
>     //    |      |  |          |  |  |  |     +------ expected result sequence
>     //    |      |  |          |  |  |  |     |  +--- expected result sequence 
>     //    |      |  |          |  |  |  |     |  |              for characters
>     //    |      |  |          |  |  |  |     |  |    exception info ------+ 
>     //    |      |  |          |  |  |  |     |  |    0 - no exception     |   
>     //    |      |  |          |  |  |  |     |  |    1 - out_of_range     |   
>     //    |      |  |          |  |  |  |     |  |    2 - length_error     |   
>     //    |      |  |          |  |  |  |     |  |                         |
>     //    |      |  |          |  |  |  |     |  +-----------+             |
>     //    V      V  V          V  V  V  V     V              V             V
>     TEST ("ab",  0, "c",       0, 1, 1, 'c',  "cab",         "cab",        0),

I think we should have exactly one result for each test case. It
makes sense (to me) to exercise two or more member functions on
the same line as long as they both (or all) produce the same
result (i.e., as long as the calls are equivalent).

Also, it would be nice to replace the hardwired numbers used to
represent exceptions with some descriptive enumerations, say
ENone, ERange, and ELength (and change the type from int to that
enumeration).

[...]
> template <class String>
> String& test_insert (const ITags                  which,

Is it necessary to return anything from this function? Wouldn't it
be simpler to return void?

>                      const TestCase              &cs,
>                      String                      &str,

Hmmm. Could this function create the string object from a narrow
character string passed to it?


>                      typename String::value_type *wsrc, 

Same here. Could this function widen the narrow character string
passed to it by the caller? That way the caller wouldn't need to
depend on any template arguments.

>                      bool                         test,

What does this argument mean? (Better names/comments please :)

>                      typename String::iterator   *ret_it)

Could we pass just the offset of the iterator from begin() back
to the caller (and avoid the caller having to depend on the
template arguments)?

> {
>     *ret_it = str.end ();
> 
>     if (!test && (i_num_str == which || i_iters_num_char == which))
>         return str;
> 
>     const String& ref_src = String (wsrc, cs.src_len);

Why reference to an unnamed temporary and not simply:

     const String ref_src (wsrc, cs.src_len);

> 
>     const typename String::value_type ctmp = 
>         make_char (cs.ch, (typename String::value_type*)0);

I would either introduce a convenience charT typedef, or better
yet, make it template parameter. Some compilers tend to have
problems deducing template arguments from "complex" type such
as String.

> 
>     // construct iterator
>     typename String::iterator it (std::size_t (cs.pos1) >= str.size () ? 
>                                       str.end () 
>                                     : str.begin () + cs.pos1);

If it doesn't get modified let's make the iterator const.

> 
>     switch (which) {
>         case i_ptr: {
>             if (test)
>                 return str.insert (cs.pos1, wsrc);
>             else

No need for the else after a return.

>                 return str.insert (cs.pos1, String (wsrc));
>         }
>         case i_str: {
>             if (test)
>                 return str.insert (cs.pos1, ref_src);
>             else

Same.

>                 return str.insert (cs.pos1, ref_src, 0, String::npos);
>         }
>         case i_num_ptr: {
>             if (test)
>                 return str.insert (cs.pos1, wsrc, cs.num);
>             else

Same.

>                 return str.insert (cs.pos1, String (wsrc, cs.num));
>         }
>         case i_num_str: {
>             if (test)
>                 return str.insert (cs.pos1, ref_src, cs.pos2, cs.num);
>             else 

Same.

>                 return str;
>         }
>         case i_num_char: {
>             if (test)
>                 return str.insert (cs.pos1, cs.cnt, make_char (
>                                    cs.ch, (typename  String::value_type*)0));

It helps speed up debugging (stepping through the test code) when
all temporaries/helper variables are created separately from the
call to the tested function. I suggest:

     typedef typename String::value_type charT;
     const charT charg = make_char (cs.ch, (charT*)0);
     return str.insert (cs.pos1, cs.cnt, charg);

[...]
>         case i_iters_range: {
>             // construct iterators
>             int last2 = cs.pos2 + cs.num;
> 
>             InputIter<typename String::value_type> src_first = 
>                 make_iter (wsrc + cs.pos2, wsrc + cs.pos2, wsrc + last2, 
>                         InputIter<typename String::value_type> (0, 0, 0));

We need to exercise the other iterator categories in addition
to InputIter. This will need to be done in a separate function
template so as not to bloat the test. In addition to the test
iterators, we should also exercise insert() specialized on plain
pointers (char* and charT*) as well as string iterators (i.e.,
String::const_iterator).

[...]
> template <class charT, class Traits>
> void test_insert (charT, Traits*, 
>                    const ITags     which,
>                    const MemFun   *pfid, 
>                    const TestCase &cs)

As I suggested above, I think it should be possible to make this
an ordinary function. We can do all or most of the logic setting
up the right function arguments here and pass them as integer
offsets and narrow characters/strings to the template (it might
make sense to have two or more very simple templates to avoid
having to do too much work in just one figuring out what to do
with the arguments). The template would convert the integer
and narrow character arguments to iterators (if necessary) and
wide characters and invoke the appropriate string member. When
done, it would compare the result with the (widened) expected
result and either convert the returned iterator (if any) back
to an integer and pass it to the caller or check the return
value itself.

> {
>     typedef std::basic_string <charT, Traits, 
>                                std::allocator<charT> > TestString;
> 
>     static charT wstr [LLEN];
>     static charT wsrc [LLEN];
>     static charT wres [LLEN];

After making this an ordinary function this should be moved to
the template. It should also be possible to reduce the number
of these large buffers to just two.

> 
[...]
>     const TestString& res_str = 
>         test_insert (which, cs, s_str, wsrc, true, &res_it);
>     const TestString& ctl_str = 
>         test_insert (which, cs, s_res, wsrc, false, &dummy_it);

I'm not clear on why we create two strings here. Comments please?
(FWIW, I think we should only create one test string.)

> #define CALLFMAT                                                             \
>     "line %d. std::basic_string<%s, %s<%2$s>, %s<%2$s>>(%{#*s})."            \
>     "insert (%{?}%zu%{;}%{?}begin + %zu%{;}"                                 \
>     "%{?}, %{/*.*Gs}%{;}%{?}, string(%{/*.*Gs})%{;}%{?}, %zu%{;}"            \
>     "%{?}, %zu%{;}%{?}, %#c%{;}%{?}, begin + %zu%{;}%{?}, begin + %zu%{;})"
> 
> #define CALLARGS                                                             \
>     __LINE__, pfid->cname_, pfid->tname_, pfid->aname_, int (cs.str_len),    \
>     cs.str, i_num_char >= which, cs.pos1, i_num_char < which, it_off,        \
>     i_ptr == which || i_num_ptr == which, int (sizeof (charT)),              \
>     int (cs.src_len), wsrc, i_str == which || i_num_str == which,            \
>     int (sizeof (charT)), int (s_src.size ()), s_src.c_str (),               \
>     i_num_str == which, cs.pos2, i_num_ptr == which || i_num_str == which    \
>     || i_num_char == which || i_iters_num_char == which, cs.num,             \
>     i_num_char == which || i_iters_char == which                             \
>     || i_iters_num_char == which, cs.ch, i_iters_range == which, first_off,  \
>     i_iters_range == which, last_off

This complicated logic should probably stay in the non-template to
prevent code bloat. We could unconditionally format the string here
and pass the result to the template if the template has any asserts.

Martin