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: <44AEB895.8060100@roguewave.com>
Date: Fri, 07 Jul 2006 13:40:05 -0600
From: Martin Sebor <sebor@roguewave.com>
Organization: Rogue Wave Software
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: design of testuite exceptions (was: Re: svn commit: r418319 -
 /incubator/stdcxx/trunk/tests/strings/21.string.io.cpp)
References: <4D6A8407B7AC6F4D95B0E55C4E7C4C6204805F7D@exmsk.moscow.vdiweb.com>
In-Reply-To: 
 <4D6A8407B7AC6F4D95B0E55C4E7C4C6204805F7D@exmsk.moscow.vdiweb.com>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Anton Pevtsov wrote:
> Martin, I implemented the exception throwing function and updated the
> test driver (except alg_test.h) to use it.
> The new files (rw_exception.h and exception.cpp) and diff to updated
> files are here:
> http://people.apache.org/~antonp/stdcxx07072006/

Great! A few (rather lengthy) comments:

Let's try harder not to #include any headers in rw_exception.h
or other test suite headers (ideally, we'd include <testdefs.h>
and nothing else). In rw_exception.h we don't need <stdarg.h>
and we shouldn't need <new> (there's no need to make BadAlloc
visible, it's an implementation class that exists for the sole
purpose of overriding std::bad_alloc::what(); no tests should
need to know about it; they should catch the base class by
reference instead).

The character buffer in the RwTestExceptionBase class is way
too big (as is/was the one in BadAlloc in new.cpp). I would
probably go with no more than 256 bytes and either dynamically
allocate more if needed or truncate excess characters. (I would
also suggest renaming the class to just ExceptionBase for
brevity). We should also drop the _THROWS() clauses (only
bad_alloc and derived class might need to make use of exception
specification).

No virtual functions (in fact none but the most trivial functions)
defined by the test suite should be inline (to avoid making the
compiler and inliner working too hard. They should be defined in
a .cpp file instead. (FYI: there is rarely any point in defining
any virtual functions inline since even modern C++ compilers are
only exceedingly rarely able to inline them.)

Since _rw_throw_exception() is a public testsuite helper (i.e.,
one that's exposed to the tests by a declaration in a header)
its name should not be decorated with the leading underscore
(only private/hidden testsuite helper functions should be).
I would also suggest to rename the function to simply rw_throw()
(since nothing but an exception can be thrown in C++ ;-)

In exception.cpp (and the rest of the test driver's .cpp files)
we should #include only the <xxx.h> headers, not their <cxxx>
counterparts (i.e., <stddef.h> instead of <cstddef>). The latter
sometimes define only symbols specified by the C++ standard while
the former usually defined other names as well (e.g., POSIX or
C99 names).

Going forward, I would prefer to use the driver's notification
facilities to "log" noteworthy events rather than writing text
messages to stderr. I realize that new.cpp does use stderr for
logging I'd just as soon not make it a generally available
feature exposed in a new API (i.e., via the logtostderr
argument). Let's keep the stderr logging in new.cpp for now
and decide how to deal with it later (ideally it would be
controlled via one of the driver's existing command line
options such as -v, --trace, or a new one such as --log).

In rw_allocator.h I don't think it's appropriate to be relying
on a library class (__rw_synchronized) for any essential test
suite functionality. If we need to make a piece of the driver
thread-safe we should implement it independently of the
library. (Out of curiosity, why does SharedAlloc need to be
thread-safe?)

Finally, remember to decorate exported names with the
_TEST_EXPORT macro.

Martin