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: <44286E02.4050802@roguewave.com>
Date: Mon, 27 Mar 2006 15:58:10 -0700
From: Martin Sebor <sebor@roguewave.com>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20050920
MIME-Version: 1.0
To: stdcxx-dev@incubator.apache.org
Subject: Re: test for lib.string.insert
References: <4D6A8407B7AC6F4D95B0E55C4E7C4C6203EE9F1C@exmsk.moscow.vdiweb.com>
In-Reply-To: 
 <4D6A8407B7AC6F4D95B0E55C4E7C4C6203EE9F1C@exmsk.moscow.vdiweb.com>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Anton Pevtsov wrote:
> 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.

I see. That is one possible way of verifying the requirements.
I don't think it's necessarily the most robust approach, however.
Suppose the charT* overload does behave as specified but the
basic_string overload fails some (or all) assertions. Should the
test of the charT* overload not fail? I believe it must! Otherwise
a failure in one component of the library ends up masking failures
in others.

The "correct" way to interpret the descriptions in the standard
is to substitute the behavior of the referenced function (the
basic_string overload in our case) for the Returns clause of the
referencing function (the charT* overload) and implement the test
for one without relying on another. The reason why the standard
uses this form of description is to avoid duplicating the text
all over the place.

Note that there is an important difference between a Returns and
a Effects clause.

A Returns clause is intended to describe the final outcome but
not necessarily the observable side-effects of computing the
result. For example, a Returns clause for the function template
foo(x) that specifies that it return the result of bar(x) (where
bar is another function template) shouldn't be interpreted as
requiring foo() to actually call bar() (which may be detectable
by a program that explicitly specializes bar() on the type x).
Rather, it should be interpreted as requiring foo(x) to return
the same value as what bar(x) is required to return when called
with the same value of x.

An Effects clause, on the other hand, describes the requirements
on the behavior of the function, including any observable side
effects, including calls to other functions. If the behavior of
foo(x) above were specified as returning bar(x) in terms of an
Effects clause, foo would be required to call bar(x) to compute
the result.

(As with everything else, there might be unintended exceptions
to the Returns/Effects rule that should be fixed in the standard
text.)

> 
> 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.

I don't think this is quite obvious from the code :) so it should be
prominently documented. But instead of documenting I would prefer to
avoid doing it in the first place, certainly if you agree with what
I said above and change the test to avoid the testing of one function
against the result of another.

[...]
> 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.

Hmmm, I'm afraid I missed this. I see this test follows the same
pattern as 21.string.replace.cpp.

I agree that it's useful to use the same test case (i.e., the same
element of the test_case[] array) to exercise the behavior of two
or more closely related overloads of the same function, but only
so long as all such calls are equivalent (i.e., the arguments denote
the same values or ranges and the major effects and the results of
the call are the same).

I don't want to see the same test case used to exercise unrelated
or even related functions when each produces a different result.

For example, I think it's perfectly fine for all of the following
(hypothetical) functions to be exercised with the same test case
because the arguments and the effects of all the functions are
equivalent:

     string::insert (pos_type, const char*);
     string::insert (pos_type, const string&);
     string::insert (iterator, const char*);
     string::insert (iterator, const string&);

but I wouldn't want the set to include:

     string::insert (pos_type, charT, size_type);

even in a test case where all the functions were called with such
arguments so as to make the calls equivalent in their results.

I would be less opposed to exercising these two functions with the
same test case:

     string::insert (iterator, InputIterator, InputIterator);
     string::insert (pos_type, const string&, size_type, size_type);

since the second could be considered just a special case of the
first in that their calls are equivalent if the iterators in calls
to the first overload are set to point to the substring denoted by
the third and fourth argument in calls to the second insert.

All that being said, I would like to keep the same structure for
all test cases. I just want each contiguous range of TEST() macros
to expand to exactly one set of equivalent function calls, and I
want each function in each set to be exercised independently of
the others. I.e., iterate over all test cases and run all tests
exercising the first overload of insert, then iterate again and
run those for the next overload, and so on.

In summary, let's please change both the insert as well as the
replace tests according to these guidelines. Are there any other
tests that we should revisit?

> 
> 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.

A reference is just a pointer, and a pointer is just an offset :)
So if we really want to verify that the returned reference refers
to the tested string object (IMO, it's hard to imagine that it
wouldn't), we could have the testing function template return the
difference between the address of the string object and the address
of the returned reference. The expected result is 0, anything else
means the returned reference is not valid.

> 
> 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.

Those will be good to add in any case. Some compilers have problems
deducing dependent types (such as String::value_type) from the types
of template arguments (such as String) used in function calls (i.e.,
in otherwise non-deducible contexts).

> 
[...]
> 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.

I meant: "add comments to the code please :)" But I don't suppose
creating the two strings will be necessary anymore after the changes
above are implemented.


> 
> 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.

Another important area to exercise (so far we've been ignoring it)
is the exception safety of all these calls. I suggest you look at
the 23.vector.modifiers.cpp test to see how it's done there. We
will need to modify all the string tests to do something similar
(although not as complicated since the charT ctors or assignment
operator can't throw).

Thanks
Martin