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.

There are few outstanding issues here that we need to resolve before
committing this patch...


[...]
@@ -58,150 +64,129 @@
[...]
-#ifndef _RWSTD_NO_WCHAR_T
+    // the value that we will be formatting
+    int value_;

I suggest to make the type of the variable double to make it possible
to exercise the formatting of the fractional part of the value (for
floating point types).

- struct WideBuf: std::wstreambuf {
-        int_type overflow (int_type c) { return c; }
-    } wb;
+    // holds the narrow/wide character representation of value_ and
+    // the number of used 'charT' in each buffer.
+    struct result {
+        char    ncs_ [BufferSize];
+        std::char_traits<char>::off_type nlen_;

This member doesn't appear to be used in the test.

-#endif // _RWSTD_NO_WCHAR_T
+        wchar_t wcs_ [BufferSize];
+        std::char_traits<wchar_t>::off_type wlen_;

Also unused.

+    };
- struct Ios: std::ios {
-        Ios () { this->init (0); }
-    } io;
+    result bool_;
+    result long_;
+    result ulong_;
+    result llong_;
+    result ullong_;
+    result double_;
+    result ldouble_;
+    result pointer_;

FYI: the naming convention we've been trying to follow for names
referring to fundamental types is the one used by the C numeric
limits macros: i.e., "dbl" for double and "ldbl" for long double.
For pointers, we've been using "ptr." It comes in handy when using
the preprocessor when parametrizing functions on these types (and
when interacting with the test driver and command line options).

[...]
+#define countof(x) (sizeof (x) / sizeof (*x))

Since you seem to like it so much ;-) we might as well move this
macro to some central test suite header (and rename it according
to the naming convention).

[...]
+    for (int i = 0; i != rw_opt_nloops; ++i) {
- io.width (i % 16);
+        // fill in the value and results for this locale
+        const MyNumData& data = my_num_data [i % nlocales];
- // exercise postive and negative values
-        const int ival = i & 1 ? -i : i;
-
+        // construct a named locale and imbue it in the ios object
+        // so that the locale is used not only by the num_put facet
+        const std::locale loc =
+            rw_opt_shared_locale ? data.locale_
+                                 : std::locale (data.locale_name_);
         if (test_char) {
             // exercise the narrow char specialization of the facet
const std::num_put<char> &np =
                 std::use_facet<std::num_put<char> >(loc);
- const std::ostreambuf_iterator<char> iter (&sb);
+            nio.imbue (loc);

Calling imbue() on the ios object before calling rdbuf() on it prevents
the same locale from being imbued in the streambuf. Did you intend for
that to happen?

[...]
+#define TEST(sb, buf, cmp, io, p, fill, valueT, val, charT)   \
+    sb.pubsetp (buf, countof (buf));                          \
+    io.rdbuf (&sb);                                           \

Is the call to rdbuf() necessary for every test or can it be done
just once per thread? (Preferably before calling imbue() on the ios
object.)

+    *p.put (std::ostreambuf_iterator<charT>(&sb),             \
+            io, fill, (valueT)(val)) = charT ();              \
+    RW_ASSERT (!io.fail ());                                  \
+    RW_ASSERT (!rw_strncmp (buf, cmp));
- case put_long:
-                np.put (iter, io, ' ', long (ival));
-                break;
+#define TEST_N(o, t, v)                                       \
+    TEST(nsb, ncs, o.ncs_, nio, np, ' ', t, v, char)
- case put_ulong:
-                np.put (iter, io, ' ', (unsigned long)ival);
-                break;
-
+ TEST_N (data.bool_, bool, data.value_ != 0); + TEST_N (data.long_, long, data.value_);
+    TEST_N (data.ulong_, unsigned long, data.value_);

I note you've changed the test from invoking one num_put member
per iteration to invoking all members each iteration. I'm curious
about your rationale for the change? (I don't necessarily disagree
with the approach, just wondering what if anything led you to make
the change.) FWIW, the one advantage to sticking with the original
testing strategy is that it would let us eliminate the call to
pubsetp() and (possibly) do away with the macros.

[...]
@@ -266,23 +229,126 @@
/**************************************************************************/ +
 static int
 run_test (int, char**)
 {
[...]
+    const char* const possible_locale_options[] = {
+        locale_list, "C\0", 0
+    };

Same applies as in this post:
http://www.mail-archive.com/stdcxx-dev@incubator.apache.org/msg04274.html

[...]
+#define SETUP(sb, buf, io, p, fill, valueT, val, charT, loc)  \
+    sb.pubsetp (buf, countof (buf));                          \
+    io.rdbuf (&sb);                                           \
+    *p.put (std::ostreambuf_iterator<charT>(&sb),             \
+             io, fill, (valueT)(val)) = charT ();             \
+    rw_assert (!io.fail (), __FILE__, __LINE__,               \

I don't think rw_assert() is appropriate here. If the facet fails
to format the "master" value into the buffer here the rest of the
test is most likely hosed anyway and shouldn't even be attempted.
Wouldn't rw_fatal() be a better choice?

Also, if you'd like to get rid of the macro (I would :), it should
be possible to move the call to rdbuf() outside the loop and verify
that the formatting was successful only once per iteration.


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

Assuming we change rw_locales() to always return at least "C" this
should never happen, right? (I.e., we'll be able to eliminate the
call to rw_fatal() after the change.)

Martin

Reply via email to