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.num.put.mt.cpp to validate results
Date Mon, 06 Aug 2007 23:07:03 GMT
Travis Vitek wrote:
> Attached is a patch to enhance the num_put facet mt test. Threads
> verify that the values they put compare equal to those put in the
> primary thread.

I'm afraid the patch didn't apply cleanly for me (probably due
to whitespace issues). Try attaching it to your post to see if
it makes it past ezmlm. If that doesn't work, unless you can
put it up on your web page somewgere you might need to open
a Jira for the enhancement and attach the patch to it.

A few comments on the changes follow inline...

> 
> 2007-08-06	Travis Vitek	<vitek@roguewave.com>
> 
> 	* 22.locale.num.put.mt.cpp: Added structure for MyIos,
> MyStreambuf
> 	and CnumData types to simplify testing.

This should be something like:

	* 22.locale.num.put.mt.cpp (MyIos, MyStreambuf, CnumData):
	Added structures to simplify testing.

[...]
> @@ -58,8 +60,98 @@
>  static std::size_t
>  nlocales;
>  
> +struct CnumData
> +{
> +    enum { BufferSize = 32 };
> +
> +    // name of the locale the data corresponds to
> +    const char* locale_name_;
> +
> +    // optinally set to the named locale for threads to share
> +    std::locale locale_;
> +
> +    // holder for original value and character representation
> +    template <typename T>

We've been discussing starting to rely on member templates in
the (near) future but we haven't decided to start yet. I suggest
to avoid using them until we've collectively decided it's okay
to do so. Here, you should be able to use a discriminated union
to get the same effect, or even simpler, use a double for all
data and convert it to the destination type as needed.

> +    struct result
> +    {
> +        result()
> +            : flags_(std::ios::dec)

We need a space before the open paren and the open curly goes on
the same line here (and in all nested definitions). The only place
we drop it is in namespace-scope definitions :)

> +        {
> +            memset(nrep_, 0, sizeof nrep_);
> +            memset(wrep_, 0, sizeof wrep_);

memset() must be qualified with std:: here (and followed by
a single space), although I don't think zeroing out the whole
buffer should really be necessary.

> +        }
> +
> +        T value_; // the original value
> +        std::ios::fmtflags flags_; // flags used

FWIW, justifying the data members to the same column it makes the
code more readable, like so:

+        T                  value_;   // the original value
+        std::ios::fmtflags flags_;   // flags used

> +
> +        // holds the narrow/wide character representation of value_
> +        char    nrep_[BufferSize]; 
> +        wchar_t wrep_[BufferSize];

Space missing before the open bracket.

[...]
> +    result<void*> ptr_;
> +
> +} num_put_data[MAX_THREADS];

Space missing before the open bracket.

[...]
> @@ -69,54 +161,43 @@
[...]
> -#ifndef _RWSTD_NO_WCHAR_T
> +// these macros should probably assert goodbit

Let's assert it if it's the right thing to do.

>  
> -    struct WideBuf: std::wstreambuf {
> -        int_type overflow (int_type c) { return c; }
> -    } wb;
> +#define TEST_PUT(cType, vType, field, value, fill, flag)            \

This is one big macro! It seems to me that it should only be
necessary to create all the objects (MyIos, MyStreambuf, etc.)
before entering the case/switch statement, and assert after
exiting it. Even better, it should be possible to create most
of them once per thread, and simply reset and reuse them in
each iteration. The only object that needs to be constructed
from scratch for every iteration is the iterator. Remember,
the loop needs to as tight and as efficient as possible in
order to maximize the contention in the tested function.

> +    {                                                               \
> +        MyIos<cType, std::char_traits<cType> > io;                  \
> +        io.imbue(loc); io.flags(flag); io.width(data->width_);      \
> +                                                                    \
> +        cType chars[CnumData::BufferSize];                          \
> +        memset(chars, 0, sizeof chars);                             \

memset() must be qualified with std:: here if you must zero out
all the elements. Alternatively, you can initialize the array
while defining it, like so:

+        cType chars [CnumData::BufferSize] = { cType () };

Btw., the name cType is misleading (there's a ctype facet). If
it refers to the same thing as charT I suggest calling it charT.

> +                                                                    \
> +        MyStreambuf<cType, std::char_traits<cType> > sb             \
> +           (chars, chars + CnumData::BufferSize);                   \
> +                                                                    \
> +        const std::ostreambuf_iterator<cType> iter (&sb);           \
> +                                                                    \
> +        np.put(iter, io, fill, value);                              \
> +        rw_assert(0 == memcmp(field, chars, sizeof chars),          \

We can't be using rw_assert() in a thread function (the driver
isn't thread safe yet, and if it was it would unnecessarily
synchronize the threads). Use the RW_ASSERT() macro instead.

[...]
>      for (int i = 0; i != rw_opt_nloops; ++i) {
>  
> -        // save the name of the locale
> -        const char* const locale_name = locales [i % nlocales];
> +        // fill in the value and results for this locale
> +        const CnumData* data = num_put_data + (i % nlocales);

If data isn't modified you should declare it const:

+ const CnumData* const data = num_put_data + (i % nlocales);

[...]
> @@ -131,75 +212,62 @@
[...]
> +                TEST_PUT_NARROW(bool, data->bool_);

Space missing before the open paren ab

> @@ -273,12 +343,124 @@
[...]
> +#if 1
> +#  define PUT_NARROW(it)                                           \
> +    PUT(char, it.nrep_, it.value_, ' ', it.flags_, np)
> +#else

Did you leave this block here on purpose?

[...]
> +        catch(...) {

Space missing before the open paren.

> +            // skip over bad locale
> +            //rw_warn(0, 0, 0, "unable to create locale '%s'", name);

I suggest you remove the commented out code just so someone doesn't
accidentally uncomment it (and cause failures due to rw_warn() not
being thread safe).

> +        }
> +
>          if (nlocales == maxinx)
>              break;
>      }
> @@ -290,6 +472,13 @@
>               rw_opt_nloops, 1 != rw_opt_nloops,
>               int (nlocales), "%#s", locales);
>  
> +    // avoid divide by zero in thread if there are no locales to test
> +    if (nlocales < 1)
> +    {

The paren goes on the same line as the if :)

> +        rw_fatal(nlocales != 0, __FILE__, __LINE__,
> +            "failed to create one or more usable locales");

The argument list continued on the second line should be aligned
with the argument list on the first line.

Martin

Mime
View raw message