You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stdcxx.apache.org by Travis Vitek <tv...@quovadx.com> on 2007/08/10 23:02:13 UTC
[PATCH] Update test 22.locale.time.put.mt.cpp to validate results [take 2]
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.
2007-08-10 Travis Vitek <vi...@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.
(main): Add command line options for specifying number
of locales, locale names, and locale usage.
Re: [PATCH] Update test 22.locale.time.put.mt.cpp to validate results
[take 2]
Posted by Martin Sebor <se...@roguewave.com>.
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.
>
> 2007-08-10 Travis Vitek <vi...@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.
> (main): Add command line options for specifying number
> of locales, locale names, and locale usage.
Committed thus: http://svn.apache.org/viewvc?view=rev&rev=565168
I have a couple of minor comments on the patch (see below). I've
also made a few trivial fixups in the next change to the test:
http://svn.apache.org/viewvc?view=rev&rev=565173
>
>
> ------------------------------------------------------------------------
>
> Index: 22.locale.time.put.mt.cpp
> ===================================================================
> --- 22.locale.time.put.mt.cpp (revision 564268)
> +++ 22.locale.time.put.mt.cpp (working copy)
> @@ -32,14 +32,14 @@
> #include <iterator> // for ostreambuf_iterator
> #include <locale> // for locale, time_put
>
> -#include <cstring> // for strlen()
> +#include <cstring> // for strlen ()
As a general rule, please try to avoid unnecessary whitespace
or formatting changes (including code movement). They needlessly
increase the effort required to review patches.
[...]
> @@ -160,19 +214,117 @@
> static int
> run_test (int, char**)
> {
[...]
> + try {
> + const std::locale loc (data.locale_name_);
> +
> + // initialize tm with random but valid values
> + data.time_.tm_sec = ++j % 61;
> + data.time_.tm_min = ++j % 60;
> + data.time_.tm_hour = ++j % 12;
> + data.time_.tm_wday = ++j % 7;
> + data.time_.tm_mon = ++j % 12;
> + data.time_.tm_mday = ++j % 31;
> + data.time_.tm_yday = ++j % 366;
> + data.time_.tm_year = ++j;
> +
> + const char cvtspecs[] = "aAbBcCdDeFgGhHIjmMnprRStTuUVwWxXyYzZ%";
These statements can't throw and don't need to be in the try block.
In general, it's best to keep the try block as small as necessary
to guard only the statements that can throw.
This might be a good rule to include in our coding guidelines (when
we finally create the document! :)
Martin
Re: [PATCH] Update test 22.locale.time.put.mt.cpp to validate results
[take 2]
Posted by Martin Sebor <se...@roguewave.com>.
Travis Vitek wrote:
>
>
> Travis Vitek wrote:
>> Martin Sebor wrote:
>>> What do you think about this: let's change rw_locales() to always
>>> return a list of names that starts with "C". That way callers that
>>> don't want to exercise the "C" locale can simply skip past it while
>>> others will be guaranteed to exercise the classic locale.
>>>
>> Sounds good. If we all agee, then I'll make up a patch for rw_locales()
>> and for each of the tests that I added last week.
>>
>> I'm thinking that rw_locales should take a bool that indicates the "C"
>> locale should be included at the head of the list. Ideally the default
>> value would be true, but for compatibility it should be set to false.
>> Hmmm. I'm thinking false and the tests that want the new
>> behavior can be
>> updated later.
>>
>> I will also need to add support for a command line option to enable or
>> disable this behavior. I'm thinking --use-c-locale=# or --no-c-locale#
>> depending on what we decide for the default value mentioned above. Does
>> that sound okay?
>
> Ugh. What if the user provides both options --locales=en_US,es_MX and
> --use-c-locale? Should we attempt to add the "C" locale to the user
> supplied locale list, or should this only be something that is in effect
> when the user doesn't use --locales?
I wouldn't. If the user says --locales=en_US,es_MX then I'd trust
them to mean en_US and es_MX and not en_US, es_MX, and "C" :)
>
> The second option is weird, but it makes the most sense. The user could
> easily just write --locales=en_US,es_MX,C if that is what they wanted.
Exactly!
Martin
RE: [PATCH] Update test 22.locale.time.put.mt.cpp to validate results [take 2]
Posted by Travis Vitek <tv...@quovadx.com>.
Travis Vitek wrote:
>
>Martin Sebor wrote:
>>
>>What do you think about this: let's change rw_locales() to always
>>return a list of names that starts with "C". That way callers that
>>don't want to exercise the "C" locale can simply skip past it while
>>others will be guaranteed to exercise the classic locale.
>>
>Sounds good. If we all agee, then I'll make up a patch for rw_locales()
>and for each of the tests that I added last week.
>
>I'm thinking that rw_locales should take a bool that indicates the "C"
>locale should be included at the head of the list. Ideally the default
>value would be true, but for compatibility it should be set to false.
>Hmmm. I'm thinking false and the tests that want the new
>behavior can be
>updated later.
>
>I will also need to add support for a command line option to enable or
>disable this behavior. I'm thinking --use-c-locale=# or --no-c-locale#
>depending on what we decide for the default value mentioned above. Does
>that sound okay?
Ugh. What if the user provides both options --locales=en_US,es_MX and
--use-c-locale? Should we attempt to add the "C" locale to the user
supplied locale list, or should this only be something that is in effect
when the user doesn't use --locales?
The second option is weird, but it makes the most sense. The user could
easily just write --locales=en_US,es_MX,C if that is what they wanted.
In either case it looks like I might need to make a function in
test/rw_locale.h that provides some documentation and sets a flag that
is used by rw_locales().
>
>>Martin
>>
>>
>
Re: [PATCH] Update test 22.locale.time.put.mt.cpp to validate results
[take 2]
Posted by Martin Sebor <se...@roguewave.com>.
Travis Vitek wrote:
>
>
> Martin Sebor wrote:
>> Now that I've committed the patch... I have a question about the bit
>> below that I noticed too late:
>>
>> [...]
>>> @@ -160,19 +214,117 @@
>>> static int
>>> run_test (int, char**)
>> [...]
>>> + const char* const possible_locale_options[] = {
>>> + locale_list, "C\0", 0
>>> + };
>> Is the purpose of this code to exercise the "C" locale in addition
>> to all the named locales returned from rw_locales()?
>>
>> If so, it's a valuable enhancement to the test since the base facet
>> (i.e., time_put as opposed to time_put_byname) wasn't necessarily
>> being exercised by the test before this change. Good catch!
>>
> Well that isn't the intent and I don't think that is what it does.
>
> There was some discussion [http://tinyurl.com/2jkqmd] about what to do
> in the case where the no valid locale could be located, either because
> the user specified invalid locale names, or because the system didn't
> have any valid locales. So the test attempts to use the provided or
> enumerated list of locales, and if none of them could be loaded, the "C"
> locale will be used. If, after attempting to load the "C" locale, no
> locale could be loaded a rw_fatal diagnostic will be issued.
I see. I think we should keep it simple. If the user specifies
--locales="foo bar" on the command line and neither foo nor bar
is valid, I'd just give a warning (or an error if you prefer)
that the locales were not valid and be done with it. I'd rather
not try to build too much "intelligence" into the tests.
[...]
> As suggested below, we could modify rw_locales method to optionally
> insert the "C" locale to the head of the locale list. This would ensure
> that the test always had at least one valid locale when invoked without
> the --locales option. It would also nicely generate an rw_fatal
> diagnostic if the user didn't provide a list of valid locales.
Yes, let's do that.
>
[...]
> Sounds good. If we all agee, then I'll make up a patch for rw_locales()
> and for each of the tests that I added last week.
>
> I'm thinking that rw_locales should take a bool that indicates the "C"
> locale should be included at the head of the list. Ideally the default
> value would be true, but for compatibility it should be set to false.
> Hmmm. I'm thinking false and the tests that want the new behavior can be
> updated later.
I wouldn't worry about compatibility. The function already returns
the "C" locale on all platforms, we'd just be making it the first
one on all of them (you should probably skip it the next time it
comes up).
>
> I will also need to add support for a command line option to enable or
> disable this behavior. I'm thinking --use-c-locale=# or --no-c-locale#
> depending on what we decide for the default value mentioned above. Does
> that sound okay?
I don't see the need for that. The user can specify the exact list
of locales using the --locales option.
Martin
RE: [PATCH] Update test 22.locale.time.put.mt.cpp to validate results [take 2]
Posted by Travis Vitek <tv...@quovadx.com>.
Martin Sebor wrote:
>
>Now that I've committed the patch... I have a question about the bit
>below that I noticed too late:
>
>[...]
>> @@ -160,19 +214,117 @@
>> static int
>> run_test (int, char**)
>[...]
>> + const char* const possible_locale_options[] = {
>> + locale_list, "C\0", 0
>> + };
>
>Is the purpose of this code to exercise the "C" locale in addition
>to all the named locales returned from rw_locales()?
>
>If so, it's a valuable enhancement to the test since the base facet
>(i.e., time_put as opposed to time_put_byname) wasn't necessarily
>being exercised by the test before this change. Good catch!
>
Well that isn't the intent and I don't think that is what it does.
There was some discussion [http://tinyurl.com/2jkqmd] about what to do
in the case where the no valid locale could be located, either because
the user specified invalid locale names, or because the system didn't
have any valid locales. So the test attempts to use the provided or
enumerated list of locales, and if none of them could be loaded, the "C"
locale will be used. If, after attempting to load the "C" locale, no
locale could be loaded a rw_fatal diagnostic will be issued.
>
>That said, I'm not quite happy with how this solution is "grafted"
>on to the current general mechanism we use to obtain the list of
>locales to test. First, the list of locales the test says (in the
>call to rw_info()) it exercises doesn't include this locale.
>
The list of locales displayed is the list of locales that are actually
used. If no valid provided or enumerated locale could be created, the
"C" locale would be used. In that case the rw_info diagnostic would
indicate that the "C" locale was the only locale used.
As suggested below, we could modify rw_locales method to optionally
insert the "C" locale to the head of the locale list. This would ensure
that the test always had at least one valid locale when invoked without
the --locales option. It would also nicely generate an rw_fatal
diagnostic if the user didn't provide a list of valid locales.
>Second,
>there's no way to disable it using the --locales option.
>
Agreed. As is, it is only a fallback. If we modified the behavior as
suggested, then we should make an option to enable or disable it.
>Finally,
>it's a chunk of code that will likely end up being repeated in all
>the locale tests and so it's a perfect candidate for an enhancement
>to the test driver.
>
Yes, it will get repeated. Notice it was repeated in the three tests I
wrote last week. Ugh.
>
>What do you think about this: let's change rw_locales() to always
>return a list of names that starts with "C". That way callers that
>don't want to exercise the "C" locale can simply skip past it while
>others will be guaranteed to exercise the classic locale.
>
Sounds good. If we all agee, then I'll make up a patch for rw_locales()
and for each of the tests that I added last week.
I'm thinking that rw_locales should take a bool that indicates the "C"
locale should be included at the head of the list. Ideally the default
value would be true, but for compatibility it should be set to false.
Hmmm. I'm thinking false and the tests that want the new behavior can be
updated later.
I will also need to add support for a command line option to enable or
disable this behavior. I'm thinking --use-c-locale=# or --no-c-locale#
depending on what we decide for the default value mentioned above. Does
that sound okay?
>Martin
>
>
Re: [PATCH] Update test 22.locale.time.put.mt.cpp to validate results
[take 2]
Posted by Martin Sebor <se...@roguewave.com>.
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.
Now that I've committed the patch... I have a question about the bit
below that I noticed too late:
[...]
> @@ -160,19 +214,117 @@
> static int
> run_test (int, char**)
[...]
> + const char* const possible_locale_options[] = {
> + locale_list, "C\0", 0
> + };
Is the purpose of this code to exercise the "C" locale in addition
to all the named locales returned from rw_locales()?
If so, it's a valuable enhancement to the test since the base facet
(i.e., time_put as opposed to time_put_byname) wasn't necessarily
being exercised by the test before this change. Good catch!
That said, I'm not quite happy with how this solution is "grafted"
on to the current general mechanism we use to obtain the list of
locales to test. First, the list of locales the test says (in the
call to rw_info()) it exercises doesn't include this locale. Second,
there's no way to disable it using the --locales option. Finally,
it's a chunk of code that will likely end up being repeated in all
the locale tests and so it's a perfect candidate for an enhancement
to the test driver.
What do you think about this: let's change rw_locales() to always
return a list of names that starts with "C". That way callers that
don't want to exercise the "C" locale can simply skip past it while
others will be guaranteed to exercise the classic locale.
Martin