stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anton Pevtsov" <Ant...@moscow.vdiweb.com>
Subject RE: test for lib.string.insert
Date Mon, 27 Mar 2006 15:15:22 GMT
Ok, I'll update the test for insert according your notes.
But just for the clarification: in this test I followed the logic
described below.

Consider to possible situations using the examples:

1. Test for insert (size_type pos, const charT* s). 
The standard talks that this insert version returns insert (pos,
basic_string<charT, traits, allocator> (s)). So I call the test function
with the "test" flag set to true to get the result of  the insert (pos,
s) call (res_str) and after that I call the test function with "test"
set to false to get the result of the insert (pos, basic_string (s)) -
(ctl_str). I compare the res_str and ctl_str to verify the method
correctness. The result strings from the test case structure isn't used.
Each test, which exercises the insert method version which returns
according to the standard should be equivalent to another insert()
version call, follows the way described above.

2. Test for insert (size_type pos1,  basic_string<charT, traits,
allocator>& str, size_type pos2, size_type n) or onsert (iterator p,
size_type n, charT c). 
The standard describes the results of these methods without referencing
to another insert () versions. (Or in other words these methods are not
evaluated through others according to the standard). So I call the test
function with the "test" flag set to true to get the result of  the
testing method (res_str). After that I make the call to test function
with "test" set to false just for consistence - it just returns the
string passed as a parameter (ctl_str). And I compare the res_str and
ctl_str, which is the string built from the test case structure res, to
verify the method correctness. So here I use the result strings from the
test case structure.
Each test, which exercises the insert method version which doesn't
evaluates according to the standard through any other insert() version
follows the way described above.

Martin Sebor wrote:
>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).

This is the key place. Yes, it is possible to have only one result and
use each test case to exercise the set of functions with equal results
only. As a result we will have two (or more) different set of the test
cases (where the num and cnt will be joined to one field, ch will be set
to -1 to differ the test cases, etc). 
All tests I've ported use each test case to exercise all possible
overloads - the opposite way. I can change them too, but I think it will
be useful to get the complete test for insert before.

Martin Sebor wrote:
> Is it necessary to return anything from this function? Wouldn't it be
simpler to return void?
Hm. If we return void we will lose the reference returned by the
insert() call. I thinks it is necessary to exercise the output.

Martin Sebor wrote:
> Hmmm. Could this function create the string object from a narrow
character string passed to it?

Yes, but this will require charT and Traits template parameters.

Martin Sebor wrote:
> What does this argument mean? (Better names/comments please :)

This is a flag that shows what insert version call. If set to true the
testing insert version will be called, if false the version through
which the testing one is evaluated will be called, if any.
I'll add the comments.

Martin Sebor wrote:
> 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)?

Yes, this is more useful. I'll update the test on this way.

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

See my comments in the begin, please.

Martin Sebor wrote:
> I forgot to mention: the test also needs to exercise the correctness
of inserting substrings of the controlled sequence into itself, i.e.,
things like:
>
>     std::string s ("abc");
>     s.insert (0, s.c_str ());
>
> or
>
>    s.insert (s.begin (), s.begin (), s.begin () + s.size ());
>
> etc.

Yes, this is really important. I'll add this test cases.


Thanks,
Anton Pevtsov


-----Original Message-----
From: Martin Sebor [mailto:sebor@roguewave.com] 
Sent: Saturday, March 25, 2006 00:26
To: stdcxx-dev@incubator.apache.org
Subject: Re: test for lib.string.insert


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


Mime
View raw message