incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: [PATCH] Update test 22.locale.time.put.mt.cpp to validate results
Date Wed, 08 Aug 2007 00:19:45 GMT
Travis Vitek wrote:
> Attached is a patch to enhance the time_put facet mt test. Threads
> verify that the values they put compare equal to those put in the
> primary thread.

It looks like Outlook is even more braindead than I thought. It
encoded the plain text attachment in base64, so there's no easy
way to comment on it. Sigh. I had to copy the patch to an email
and send it to myself to get the ticks ('>') so that my comments
wouldn't get mixed up with the patch. Travis, you need to get
yourself a decent mailer :) Anything but Outlook.

> 
> 2007-08-07	Travis Vitek	<vitek@roguewave.com>
> 
> 	* 22.locale.time.put.mt.cpp (MyIos, MyStreambuf, MyTimeData):
>       Added structures to simplify testing.
> 	(run_test): Build table of in/outputs for verification in test
>       threads. 
>       (thread_func): Assert that data written matches expected.
> 
> 

 > Index: 22.locale.time.put.mt.cpp
 > ===================================================================
 > --- 22.locale.time.put.mt.cpp    (revision 562577)
 > +++ 22.locale.time.put.mt.cpp    (working copy)
 > @@ -32,7 +32,7 @@
 >  #include <iterator>   // for ostreambuf_iterator
 >  #include <locale>     // for locale, time_put
 >
 > -#include <cstring>    // for strlen()
 > +#include <cstring>    // for strlen ()
 >  #include <ctime>      // for tm
 >
 >  #include <rw_locale.h>
 > @@ -60,76 +60,124 @@
 >  static std::size_t
 >  nlocales;
 >
 > 
-/**************************************************************************/ 

 >
 > +struct MyTimeData
 > +{
 > +    enum { BufferSize = 64 };
 >
 > -extern "C" {
 > +    // name of the locale the data corresponds to
 > +    const char* locale_name_;
 >
 > -bool test_char;    // exercise time_put<char>
 > -bool test_wchar;   // exercise time_put<wchar_t>
 > +    // optinally set to the named locale for threads to share
 > +    std::locale locale_;
 >
 > +    // the time struct used to generate strings below
 > +    std::tm time_;
 >
 > -static void*
 > -thread_func (void*)
 > +    // the format specifier
 > +    char format_;
 > +
 > +    // narrow representations of time_ given the
 > +    // locale_name_ and the format_
 > +    char ncs_ [BufferSize];
 > +    std::char_traits<char>::off_type nlen_;

If nlen_ is the length of ncs_ and can't be negative its type
should be size_t, not off_type (off_type is for file offsets).

[...]
 > +    charT* pubpptr () const {
 > +        return pptr ();

It's hard for me to tell but if the called function is defined
in a context that depends on a template parameter it needs to
be qualified with this-> (the compiler isn't allowed to look
in dependent bases when parsing unqualified calls). If you're
using MSVC to test your changes you might want to run them by
a recent version of gcc as well to catch more problems.

[...]
 > +static void*
 > +thread_func (void*)
 > +{
 > +    char                                       ncs
 > [MyTimeData::BufferSize];
 > +    MyIos<char, std::char_traits<char> >       nio;
 > +    MyStreambuf<char, std::char_traits<char> > nsb;
 > +
 > +#ifndef _RWSTD_NO_WCHAR_T
 > +    wchar_t                                          wcs
 > [MyTimeData::BufferSize];

Please make sure not to exceed 78 characters per line :)

[...]
 >          // save the name of the locale
 > -        const char* const locale_name = locales [i % nlocales];
 > +        const MyTimeData& data = my_time_data[i % nlocales];

Missing space before the open bracket.

 >
 >          // construct a named locale, get a reference to the time_put
 >          // facet from it and use it to format a random time value
 >          // using a random conversion specifier
 > -        const std::locale loc (locale_name);
 > +        const std::locale loc (data.locale_name_);
 >
 >          if (test_char) {
 >              // exercise the narrow char specialization of the facet
 >
 > +            // should this be hoisted out of the loop?
 >              const std::time_put<char> &tp =
 >                  std::use_facet<std::time_put<char> >(loc);

I think it probably should. In fact, creating the locale in the
thread function exercises (much) more than the tests are designed
to exercise. That's not without value but there should be a way
to reduce the scope of the test so as to avoid constructing the
same named locales in the thread function. 22.locale.ctype.mt.cpp
makes it possible under the --shared-locale command line option.

 >
[...]
 > +
 > +            const std::char_traits<char>::off_type nlen =
 > +                nsb.pubpptr() - nsb.pubpbase();

I understand why you declared nlen to have off_type but since
pbase() is required/guaranteed to be less than or equal to pptr()
the type of nlen should be size_t. That way there's no need to
worry about it being negative somewhere else.

 > +
 > +            RW_ASSERT (nlen == data.nlen_ &&
 > +                       !memcmp (data.ncs_, ncs, nlen));

memcmp() needs to be qualified with std:: here and operator&&
should be on the same line, like so:

+            RW_ASSERT (   nlen == data.nlen_
+                       && !memcmp (data.ncs_, ncs, nlen));

[...]
 > +        // fill in the time and results for this locale
 > +        MyTimeData& data = my_time_data[nlocales];

Missing space before the open bracket.

[...]
 > +#endif // _RWSTD_NO_WCHAR_T
 > +
 > +        } catch (...) {

No code should follow the closing brace.

[...]
 > +    // avoid divide by zero in thread if there are no locales to test
 > +    if (nlocales < 1) {
 > +        rw_fatal(nlocales != 0, 0, __LINE__,
 > +                 "failed to create one or more usable locales");

There are systems with no locales installed. I think we just want
rw_warn() here, not something as severe as rw_fatal(). (And we
want a space before the open paren :)

Martin

Mime
View raw message