Re: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Martin Sebor 

Eric Lemings wrote:
 

[...]

With the following change:

Index:
/work/stdcxx/branches/4.3.x/tests/utilities/20.tuple.cnstr.cpp

===
---
/work/stdcxx/branches/4.3.x/tests/utilities/20.tuple.cnstr.cpp
(revision 675050)
+++
/work/stdcxx/branches/4.3.x/tests/utilities/20.tuple.cnstr.cpp
(working copy)
@@ -74,18 +73,20 @@

[...]

It appears that pointer values are not guaranteed to be equal when
converting between pointer types.


They must be. I see the assertions in the test but the small
program below works fine.

On an unrelated note, though: While working with the test
I found the heavy use of macros and globals quite confusing.
It makes it very difficult to find and track the tested
values, and virtually impossible to extend with new test
cases.

The approach I've found to work better is splitting up the
test into one or some other small number of worker functions
parametrized on all the arguments and expected values and
their types (if necessary) and other higher level functions,
for example one for each overload of the tested function,
with individual test cases and literal values of arguments
and expected results. You can find examples of such tests
in the tests/localization directory, such as all the
22.locale.{money,num,time}.{get,put}.cpp tests. I suggest
you follow this example in the tuple tests as well.

Martin

#include 
#include 

int main ()
{
const char* s = "string";

{
std::tuple x (s);
const char* const y = std::get<0>(x);

assert (y == s);
}
{
std::tuple x (0, s);
const char* const y = std::get<1>(x);

assert (y == s);
}
}

RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Travis Vitek 
 

Martin Sebor wrote:
>
>Eric Lemings wrote:
>>  
>> 
>>> Travis Vitek wrote:
>>>  
>>>
>>> Eric Lemings wrote:
> Travis Vitek wrote:
>
> The tuple is holding the original pointer (not a copy), so I 
> think you can check the actual pointer here.

 True.  But if that assumption became invalid for whatever
 reason, the code above would still work.

 Assumptions are bad.  Robustness is good.  :)
>>>
>>> As I see it, the tuple implementation is required to hold a 
>>> copy of an object of the specified type (const char* in this
>>> case). If you don't verify the value held is indeed a copy,
>>> you are not actually verifying the requirements. This is wrong,
>>> and wrong is much worse than bad. :)
>> 
>> Question:
>> 
>> const char* s1 = "string";
>> const char* s2 = "string";
>> // s1 guaranteed to equal s2?
>
>It's unspecified. The compiler is allowed to merge strings.
>It's allowed to even go as far as to point s2 at (s1 + 1)
>in the snippet below:
>
> const char* s1 = "Xstring";
> const char* s2 = "string";
>

Just so we're clear, the case that should be happening with tuple is the
following...

  const char* s1 = "string";
  const char* s2 (s1);

  assert (s1 == s2);

The following code works just fine with both the gnu tuple
implementation and ours. I'm not sure why the Brad is seeing the
assertion failure.

  $ cat t.cpp && g++ -std=gnu++0x t.cpp && ./a.out && echo good
  #include 
  #include 

  static const char* s = "hello world";

  int main ()
  {
const std::tuple t (s);
assert (std::get<0>(t) == s);

return 0;
  }
  $ good

  $ gmake t && ./t && echo good
  gcc -c -I/amd/devco/vitek/stdcxx/4.3.x/include/ansi -D_RWSTDDEBUG
-D_RWSTD_EXT_CXX_0X -I/amd/devco/vitek/stdcxx/4.3.x/include
-I/build/vitek/4.3.0/11S/include
-I/amd/devco/vitek/stdcxx/4.3.x/tests/include -pedantic -nostdinc++ -g
-std=gnu++0x -W -Wall -Wcast-qual -Winline -Wshadow -Wwrite-strings
-Wno-long-long -Wcast-align -Wno-empty-body -Wno-parentheses t.cpp
  gcc t.o -o t -L/build/vitek/4.3.0/11S/rwtest -lrwtest11S
-L/build/vitek/4.3.0/11S/lib -lstd11S -lsupc++ -lm
  good

>Martin
>
>

RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Eric Lemings 
 

> -Original Message-
> From: Eric Lemings 
> Sent: Wednesday, July 09, 2008 3:14 PM
> To: 'dev@stdcxx.apache.org'
> Subject: RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
> include/rw/_tuple.h include/tuple 
> tests/utilities/20.tuple.cnstr.cpp 
> tests/utilities/20.tuple.creation.cpp 
> tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp
> 
>  
> 
> > -Original Message-
> > From: Martin Sebor [mailto:[EMAIL PROTECTED] 
> > Sent: Wednesday, July 09, 2008 11:10 AM
> > To: dev@stdcxx.apache.org
> > Subject: Re: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
> > include/rw/_tuple.h include/tuple 
> > tests/utilities/20.tuple.cnstr.cpp 
> > tests/utilities/20.tuple.creation.cpp 
> > tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp
> > 
> ...
> > > This can probably be changed to use a void return type, which will
> > > simplify the code further. You only really need the return 
> > type to chain
> > > assignments or to call a function on the result, none of 
> > which we should
> > > be doing.
> > 
> > Good idea! Also, the inline specifier is redundant and should
> > be removed.
> 
> A void return type causes an compile error:

Duh.  Disregard.

RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Eric Lemings 
 

> -Original Message-
> From: Martin Sebor [mailto:[EMAIL PROTECTED] 
> Sent: Wednesday, July 09, 2008 11:10 AM
> To: dev@stdcxx.apache.org
> Subject: Re: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
> include/rw/_tuple.h include/tuple 
> tests/utilities/20.tuple.cnstr.cpp 
> tests/utilities/20.tuple.creation.cpp 
> tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp
> 
...
> > This can probably be changed to use a void return type, which will
> > simplify the code further. You only really need the return 
> type to chain
> > assignments or to call a function on the result, none of 
> which we should
> > be doing.
> 
> Good idea! Also, the inline specifier is redundant and should
> be removed.

A void return type causes an compile error:

gcc -c -I/work/stdcxx/branches/4.3.x/include/ansi -D_RWSTDDEBUG
-pthread -I/work/stdcxx/branches/4.3.x/include
-I/build/stdcxx-4.3.x-15D/include
-I/work/stdcxx/branches/4.3.x/tests/include  -pedantic -nostdinc++
-std=gnu++0x -D_RWSTD_EXT_CXX_0X -g   -W -Wall -Wcast-qual -Winline
-Wshadow -Wwrite-strings -Wno-long-long -Wcast-align
/work/stdcxx/branches/4.3.x/tests/utilities/20.tuple.creation.cpp
/work/stdcxx/branches/4.3.x/include/rw/_tuple.h: In member function
'void __rw::__rw_ignore::operator=(const _TypeT&) const [with _TypeT =
double]':
/work/stdcxx/branches/4.3.x/include/rw/_tuple.h:131:   instantiated from
'__rw::__rw_tuple<_HeadT, _TailT ...>& __rw::__rw_tuple<_HeadT, _TailT
...>::operator=(__rw::__rw_tuple<_HeadU, _TailU ...>&&) [with _HeadU =
double, _TailU = const char*, _HeadT = const __rw::__rw_ignore&, _TailT
= const char*&]'
/work/stdcxx/branches/4.3.x/include/rw/_tuple.h:130:   instantiated from
'__rw::__rw_tuple<_HeadT, _TailT ...>& __rw::__rw_tuple<_HeadT, _TailT
...>::operator=(__rw::__rw_tuple<_HeadU, _TailU ...>&&) [with _HeadU =
int, _TailU = double, const char*, _HeadT = int&, _TailT = const
__rw::__rw_ignore&, const char*&]'
/work/stdcxx/branches/4.3.x/include/tuple:123:   instantiated from
'std::tuple<_TypesT>& std::tuple<_TypesT>::operator=(std::tuple<_TypesU
...>&&) [with _TypesU = int, double, const char*, _TypesT = int&, const
__rw::__rw_ignore&, const char*&]'
/work/stdcxx/branches/4.3.x/tests/utilities/20.tuple.creation.cpp:75:
instantiated from here
/work/stdcxx/branches/4.3.x/include/rw/_tuple.h:181: error:
return-statement with a value, in function returning 'void'
make: *** [20.tuple.creation.o] Error 1

Brad.

RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Eric Lemings 
 

> -Original Message-
> From: Martin Sebor [mailto:[EMAIL PROTECTED] 
> Sent: Wednesday, July 09, 2008 2:49 PM
> To: dev@stdcxx.apache.org
> Subject: Re: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
> include/rw/_tuple.h include/tuple 
> tests/utilities/20.tuple.cnstr.cpp 
> tests/utilities/20.tuple.creation.cpp 
> tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp
> 
> Eric Lemings wrote:
> >  
> > 
> >> -Original Message-
> >> From: Martin Sebor [mailto:[EMAIL PROTECTED] 
> >> Sent: Wednesday, July 09, 2008 11:10 AM
> >> To: dev@stdcxx.apache.org
> >> Subject: Re: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
> >> include/rw/_tuple.h include/tuple 
> >> tests/utilities/20.tuple.cnstr.cpp 
> >> tests/utilities/20.tuple.creation.cpp 
> >> tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp
> >>
> > ...
> >>> I think the commented out parameter name should be removed. 
> >> I don't see
> >>> this in existing code, and I personally find it a bit distracting.
> >> I agree. Without a name, it's obvious that the parameter
> >> is unused.
> > 
> > Examples in existing code:
> 
> As I said before, you can find examples of pretty much any
> style, including two space indents. Are you purposely seeking
> out these rare, obscure cases and adopting them in your code
> just to make things interesting?

Actually no, if you believe that.  Was just providing examples since
Travis could find no such usage in existing code.

Brad.

Re: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Martin Sebor 

Eric Lemings wrote:
 


-Original Message-
From: Martin Sebor [mailto:[EMAIL PROTECTED] 
Sent: Wednesday, July 09, 2008 11:10 AM

To: dev@stdcxx.apache.org
Subject: Re: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
include/rw/_tuple.h include/tuple 
tests/utilities/20.tuple.cnstr.cpp 
tests/utilities/20.tuple.creation.cpp 
tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp



...
I think the commented out parameter name should be removed. 

I don't see

this in existing code, and I personally find it a bit distracting.

I agree. Without a name, it's obvious that the parameter
is unused.


Examples in existing code:


As I said before, you can find examples of pretty much any
style, including two space indents. Are you purposely seeking
out these rare, obscure cases and adopting them in your code
just to make things interesting?



The run_test() function in tests/containers/23.vector.cons.cpp.


Not sure why the names are commented out. Maybe because
the author was intending to use them and didn't and they
got commented out to silence warnings.


Lines 56-64 in tests/containers/23.deque.modifiers.cpp.


They are there because normally, names local to each test
are declared static. In this test (and many others) they
can't be declared static because they are referenced from
template code and no all compilers find time (Sun C++ 5.3
has a bug that prevents it from finding static symbols
referenced from template code). So the /* extern */
comment is a reminder to prevent people from making them
static.


The __rw_smanip member functions in include/iomanip.


This is the { /* empty */ } comment that some style guides
suggest for non-trivial ctors with deliberately empty bodies
to indicate that the body wasn't left empty by accident when
the ctor was stubbed out early in the development of the
class. I don't feel strongly about using this style.



Who did all that?  Not me.  :)  I'm sure there are plenty more examples.


I'm sure there are.



Anyone care to search for all such cases and make it all consistent?


No. Please just adjust your code as suggested.

Thanks
Martin

RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Eric Lemings 
 

> -Original Message-
> From: Martin Sebor [mailto:[EMAIL PROTECTED] 
> Sent: Wednesday, July 09, 2008 11:10 AM
> To: dev@stdcxx.apache.org
> Subject: Re: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
> include/rw/_tuple.h include/tuple 
> tests/utilities/20.tuple.cnstr.cpp 
> tests/utilities/20.tuple.creation.cpp 
> tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp
> 
...
> > 
> > I think the commented out parameter name should be removed. 
> I don't see
> > this in existing code, and I personally find it a bit distracting.
> 
> I agree. Without a name, it's obvious that the parameter
> is unused.

Examples in existing code:

The run_test() function in tests/containers/23.vector.cons.cpp.
Lines 56-64 in tests/containers/23.deque.modifiers.cpp.
The __rw_smanip member functions in include/iomanip.

Who did all that?  Not me.  :)  I'm sure there are plenty more examples.

Anyone care to search for all such cases and make it all consistent?

Brad.

Re: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Martin Sebor 

Eric Lemings wrote:
 


-Original Message-
From: Travis Vitek [mailto:[EMAIL PROTECTED] 
Sent: Wednesday, July 09, 2008 12:28 PM

To: dev@stdcxx.apache.org
Subject: RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
include/rw/_tuple.h include/tuple 
tests/utilities/20.tuple.cnstr.cpp 
tests/utilities/20.tuple.creation.cpp 
tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp


 


Eric Lemings wrote:

Travis Vitek wrote:

Modified: 

stdcxx/branches/4.3.x/tests/utilities/20.tuple.creation.cpp
...
+rw_assert (0 == std::strcmp (s, "string"), __FILE__, 

__LINE__,

+   "s == \"string\", got false, expected true");
The tuple is holding the original pointer (not a copy), so I 

think you

can check the actual pointer here.

True.  But if that assumption became invalid for whatever reason, the
code above would still work.

Assumptions are bad.  Robustness is good.  :)

As I see it, the tuple implementation is required to hold a copy of an
object of the specified type (const char* in this case). If you don't
verify the value held is indeed a copy, you are not actually verifying
the requirements. This is wrong, and wrong is much worse than bad. :)


Question:

const char* s1 = "string";
const char* s2 = "string";
// s1 guaranteed to equal s2?


It's unspecified. The compiler is allowed to merge strings.
It's allowed to even go as far as to point s2 at (s1 + 1)
in the snippet below:

const char* s1 = "Xstring";
const char* s2 = "string";

Martin

RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Eric Lemings 
 

> -Original Message-
> From: Eric Lemings [mailto:[EMAIL PROTECTED] 
> Sent: Wednesday, July 09, 2008 12:40 PM
> To: dev@stdcxx.apache.org
> Subject: RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
> include/rw/_tuple.h include/tuple 
> tests/utilities/20.tuple.cnstr.cpp 
> tests/utilities/20.tuple.creation.cpp 
> tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp
> 
>  
...
> > 
> > As I see it, the tuple implementation is required to hold a 
> copy of an
> > object of the specified type (const char* in this case). If 
> you don't
> > verify the value held is indeed a copy, you are not 
> actually verifying
> > the requirements. This is wrong, and wrong is much worse 
> than bad. :)
> 
> Question:
> 
> const char* s1 = "string";
> const char* s2 = "string";
> // s1 guaranteed to equal s2?

With the following change:

Index:
/work/stdcxx/branches/4.3.x/tests/utilities/20.tuple.cnstr.cpp

===
---
/work/stdcxx/branches/4.3.x/tests/utilities/20.tuple.cnstr.cpp
(revision 675050)
+++
/work/stdcxx/branches/4.3.x/tests/utilities/20.tuple.cnstr.cpp
(working copy)
@@ -74,18 +73,20 @@


 #define LONG_VALUE  INT_VALUE
-#define STRING_VALUE"string"
+#define STRING_VALUEstr_value

+static const char* str_value = "string";
+
 static void
 verify_tuple (const PairTuple& pt)
 {
 rw_assert (std::get<0> (pt) == LONG_VALUE, __FILE__,
__LINE__,
"std::get<0> (pt), got %d, expected %d",
std::get<0> (pt), LONG_VALUE);
-rw_assert (0 == std::strcmp (std::get<1> (pt),
STRING_VALUE),
-   __FILE__, __LINE__,
-   "std::get<1> (pt), got %s, expected %s",
-   std::get<1> (pt), STRING_VALUE);
+rw_assert (std::get<1> (pt) == STRING_VALUE, __FILE__,
__LINE__,
+   "std::get<1> (pt), got %p \"%s\", expected %p
\"%s\"",
+   std::get<1> (pt), std::get<1> (pt),
+   STRING_VALUE, STRING_VALUE);
 }

I get the following assertions:

...
# ASSERTION (S7) (5 lines):
# TEXT: std::get<1> (pt), got 0f18d8c0 "string",
expected 0042796e "string"
# CLAUSE: [tuple.cnstr]
# FILE: 20.tuple.cnstr.cpp
# LINE: 86

# INFO (S1) (5 lines):
# TEXT: move constructor (heterogenous tuples)
# CLAUSE: [tuple.cnstr]
# FILE: 20.tuple.cnstr.cpp
# LINE: 458

# ASSERTION (S7) (5 lines):
# TEXT: std::get<1> (pt), got 0f18d8c0 "string",
expected 0042796e "string"
# CLAUSE: [tuple.cnstr]
# FILE: 20.tuple.cnstr.cpp
# LINE: 86

# INFO (S1) (5 lines):
# TEXT: copy assignment operator (heterogenous tuples)
# CLAUSE: [tuple.cnstr]
# FILE: 20.tuple.cnstr.cpp
# LINE: 480

# ASSERTION (S7) (5 lines):
# TEXT: std::get<1> (pt), got 0f18d8c0 "string",
expected 0042796e "string"
# CLAUSE: [tuple.cnstr]
# FILE: 20.tuple.cnstr.cpp
# LINE: 86

# INFO (S1) (5 lines):
# TEXT: move assignment operator (heterogenous tuples)
# CLAUSE: [tuple.cnstr]
# FILE: 20.tuple.cnstr.cpp
# LINE: 504

# ASSERTION (S7) (5 lines):
# TEXT: std::get<1> (pt), got 0f18d8c0 "string",
expected 0042796e "string"
# CLAUSE: [tuple.cnstr]
# FILE: 20.tuple.cnstr.cpp
# LINE: 86
...

It appears that pointer values are not guaranteed to be equal when
converting between pointer types.

Brad.

RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Eric Lemings 
 

> -Original Message-
> From: Travis Vitek [mailto:[EMAIL PROTECTED] 
> Sent: Wednesday, July 09, 2008 12:28 PM
> To: dev@stdcxx.apache.org
> Subject: RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
> include/rw/_tuple.h include/tuple 
> tests/utilities/20.tuple.cnstr.cpp 
> tests/utilities/20.tuple.creation.cpp 
> tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp
> 
>  
> 
> Eric Lemings wrote:
> >
> >> Travis Vitek wrote:
> >>
> >> >Modified: 
> >stdcxx/branches/4.3.x/tests/utilities/20.tuple.creation.cpp
> >...
> >> >+rw_assert (0 == std::strcmp (s, "string"), __FILE__, 
> __LINE__,
> >> >+   "s == \"string\", got false, expected true");
> >> 
> >> The tuple is holding the original pointer (not a copy), so I 
> >think you
> >> can check the actual pointer here.
> >
> >True.  But if that assumption became invalid for whatever reason, the
> >code above would still work.
> >
> >Assumptions are bad.  Robustness is good.  :)
> 
> As I see it, the tuple implementation is required to hold a copy of an
> object of the specified type (const char* in this case). If you don't
> verify the value held is indeed a copy, you are not actually verifying
> the requirements. This is wrong, and wrong is much worse than bad. :)

Question:

const char* s1 = "string";
const char* s2 = "string";
// s1 guaranteed to equal s2?

Brad.

RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Eric Lemings 
 

> -Original Message-
> From: Travis Vitek [mailto:[EMAIL PROTECTED] 
> Sent: Wednesday, July 09, 2008 12:28 PM
> To: dev@stdcxx.apache.org
> Subject: RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
> include/rw/_tuple.h include/tuple 
> tests/utilities/20.tuple.cnstr.cpp 
> tests/utilities/20.tuple.creation.cpp 
> tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp
> 
>  
> 
> Eric Lemings wrote:
> >
> >> Travis Vitek wrote:
> >>
> >> >Modified: 
> >stdcxx/branches/4.3.x/tests/utilities/20.tuple.creation.cpp
> >...
> >> >+rw_assert (0 == std::strcmp (s, "string"), __FILE__, 
> __LINE__,
> >> >+   "s == \"string\", got false, expected true");
> >> 
> >> The tuple is holding the original pointer (not a copy), so I 
> >think you
> >> can check the actual pointer here.
> >
> >True.  But if that assumption became invalid for whatever reason, the
> >code above would still work.
> >
> >Assumptions are bad.  Robustness is good.  :)
> 
> As I see it, the tuple implementation is required to hold a copy of an
> object of the specified type (const char* in this case). If you don't
> verify the value held is indeed a copy, you are not actually verifying
> the requirements. This is wrong, and wrong is much worse than bad. :)

Good point.

RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Travis Vitek 
 

Eric Lemings wrote:
>
>> Travis Vitek wrote:
>>
>> >Modified: 
>stdcxx/branches/4.3.x/tests/utilities/20.tuple.creation.cpp
>...
>> >+rw_assert (0 == std::strcmp (s, "string"), __FILE__, __LINE__,
>> >+   "s == \"string\", got false, expected true");
>> 
>> The tuple is holding the original pointer (not a copy), so I 
>think you
>> can check the actual pointer here.
>
>True.  But if that assumption became invalid for whatever reason, the
>code above would still work.
>
>Assumptions are bad.  Robustness is good.  :)

As I see it, the tuple implementation is required to hold a copy of an
object of the specified type (const char* in this case). If you don't
verify the value held is indeed a copy, you are not actually verifying
the requirements. This is wrong, and wrong is much worse than bad. :)

>
>Brad.
>

Re: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Martin Sebor 

Travis Vitek wrote:
 


Author: elemings
Date: Tue Jul  8 16:13:36 2008
New Revision: 675044

URL: http://svn.apache.org/viewvc?rev=675044&view=rev
Log:
2008-07-08  Eric Lemings <[EMAIL PROTECTED]>

STDCXX-958
* include/tuple: Second parameter in value move ctor of pair
specialization missing rvalue reference.
(make_tuple, get, relational operators): Explicitly declare
as inline functions.
(tie): Implemented.
* include/rw/_tuple.h: Fix move semantics in heterogenous move
assignment operator.
(__rw_ignore): Add assignment operator to ignore all values.
* tests/utilities/20.tuple.cnstr.cpp: Added V&V for tuple
state and invariants.  Manually inspected proper construction
of all test tuples.  Updated/corrected/added tests as necessary.
* tests/utilities/20.tuple.creation.cpp: Added simple tie()
test.
* tests/utilities/20.tuple.h: Minor stylistic changes.
* tests/utilities/20.tuple.helpers.cpp: Same.


Modified:
   stdcxx/branches/4.3.x/include/rw/_tuple.h
   stdcxx/branches/4.3.x/include/tuple
   stdcxx/branches/4.3.x/tests/utilities/20.tuple.cnstr.cpp
   stdcxx/branches/4.3.x/tests/utilities/20.tuple.creation.cpp
   stdcxx/branches/4.3.x/tests/utilities/20.tuple.h
   stdcxx/branches/4.3.x/tests/utilities/20.tuple.helpers.cpp

Modified: stdcxx/branches/4.3.x/include/rw/_tuple.h
URL: 
http://svn.apache.org/viewvc/stdcxx/branches/4.3.x/include/rw/_

tuple.h?rev=675044&r1=675043&r2=675044&view=diff
===
===
--- stdcxx/branches/4.3.x/include/rw/_tuple.h (original)
+++ stdcxx/branches/4.3.x/include/rw/_tuple.h Tue Jul  8 16:13:36 2008
@@ -174,7 +174,14 @@
};


-struct __rw_ignore { /* empty */ };
+struct __rw_ignore
+{
+template 
+inline __rw_ignore const&
+operator= (const _TypeT& /*unused*/) const {
+return *this;
+}
+};



I think the commented out parameter name should be removed. I don't see
this in existing code, and I personally find it a bit distracting.


I agree. Without a name, it's obvious that the parameter
is unused. Saying it's unused in a comment is like adding
a /* return; */ comment to the end of void functions, or
adding an /* extern */ in front of the definition of non
member function definitions. IMO, all of these represent
unnecessary redundancies that are liable to make readers
wonder about their purpose rather than providing any
helpful insight. It would be much more helpful to document
the purpose of the unusual assignment operator than the
unused argument :)



This can probably be changed to use a void return type, which will
simplify the code further. You only really need the return type to chain
assignments or to call a function on the result, none of which we should
be doing.


Good idea! Also, the inline specifier is redundant and should
be removed.




[...]

@@ -377,7 +381,7 @@
// 20.3.1.5, element access:

template <_RWSTD_SIZE_T _Index, class _Head, class... _Tail>
-_TYPENAME tuple_element<_Index, tuple<_Head, _Tail...> >::_Ref
+inline _TYPENAME tuple_element<_Index, tuple<_Head, _Tail...> >::_Ref
get (tuple<_Head, _Tail...>& __tuple)
{
typedef tuple_element<_Index, tuple<_Head, _Tail...> > _Tuple;


In the recent past Martin recommended not using the _TYPENAME macro. It
isn't hurting anything, but it could probably be removed. I know that I
no longer use it in the traits code. I also noticed the _EXPLICIT macro
above. I think that one should be added to the list. Martin?


I agree. We can start using typename and explicit in all new
code on 4.3.x and replace _TYPENAME and _EXPLICIT with the
real keywords. Ditto for _RWSTD_SPECIALIZED_FUNCTION and
_RWSTD_SPECIALIZED_CLASS.




[...]

@@ -396,59 +400,67 @@
// 20.3.1.6, relational operators:

template 
-bool operator== (const tuple<_TypesT...>& __x,
- const tuple<_TypesU...>& __y)
+inline bool
+operator== (const tuple<_TypesT...>& __x,
+const tuple<_TypesU...>& __y)
{
return _RWSTD_STATIC_CAST (const 
_RW::__rw_tuple<_TypesT...>&, __x)
   == _RWSTD_STATIC_CAST (const 
_RW::__rw_tuple<_TypesU...>&, __y);


I think there is a formatting issue here. The prevailing style is for
the operator to start of the next line, but for the operands to be lined
up on their left. As an example

  return__some_really_long_expression_1
 == __some_really_long_expression_2;


Right. We're not 100% consistent (and I can't say I'm crazy
about this style, either) but I find it more readable than
any of the alternatives I've seen.

Martin

RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-09 Thread Eric Lemings 
 

> -Original Message-
> From: Travis Vitek [mailto:[EMAIL PROTECTED] 
> Sent: Tuesday, July 08, 2008 6:04 PM
> To: dev@stdcxx.apache.org
> Subject: RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: 
> include/rw/_tuple.h include/tuple 
> tests/utilities/20.tuple.cnstr.cpp 
> tests/utilities/20.tuple.creation.cpp 
> tests/utilities/20.tuple.h tests/utilities/20.tuple.helpers.cpp
> 
>  
...
> 
> The commented unused argument names again. Also, I think the
> _RWSTD_SPECIALIZED_FUNCTION macro can be eliminated. If I remember
> correctly Martin asked that it be removed from the traits 
> implementation (which I've done).

I've been using _TYPENAME, _EXPLICIT, and _RWSTD_SPECIALIZED_FUNCTION
just to be consistent with existing code.  Is there a good reason not to
do this anymore?  (Actually I can think of one but I'll see if anyone
else can think of it and/or another.)

> 
...
> Is there any way that we could write a routine to generate a tuple and
> then test it, so as to avoid always using the same few types 
> and values
> hidden behind the *_VALUE macros? The usage would be something like
> this...
> 
>   TEST_TUPLE (1, 3.14f, 'a', "abc");

This might work for homogenous tuples where the element types can be
deduced from the values.  Not sure exactly how you would fit
user-defined (e.g. UserClass) values into it.  Also, you'd need an
expanded form for heterogenous tuples where the compatible/convertible
types would have to be explicitly specified.

For this latest update, I really wanted to just get a complete set of
tests in there however verbose they may be.

> 
> >-int i = 1;
> >-IntTuple it1 (i); _RWSTD_UNUSED (it1);
> >-const IntTuple it2 (i); _RWSTD_UNUSED (it2);
> >-ConstIntTuple ct (i); _RWSTD_UNUSED (ct);
> >-IntRefTuple rt (i); _RWSTD_UNUSED (rt);
> > 
> >-NestedTuple nt (it2); _RWSTD_UNUSED (nt);
> >+#define USER_VALUE  user_val
> 
> I'm being a nit-picker, but this seems like an awful simple 
> thing to be
> wrapping a macro around. Is there a reason to do so?

Like the other value macros, to hide the actual value being used and to
provide a single point of definition where it can be modified.

> 
> >Modified: stdcxx/branches/4.3.x/tests/utilities/20.tuple.creation.cpp
...
> >+rw_assert (0 == std::strcmp (s, "string"), __FILE__, __LINE__,
> >+   "s == \"string\", got false, expected true");
> 
> The tuple is holding the original pointer (not a copy), so I think you
> can check the actual pointer here.

True.  But if that assumption became invalid for whatever reason, the
code above would still work.

Assumptions are bad.  Robustness is good.  :)

Brad.

RE: svn commit: r675044 - in /stdcxx/branches/4.3.x: include/rw/_tuple.h include/tuple tests/utilities/20.tuple.cnstr.cpp tests/utilities/20.tuple.creation.cpp tests/utilities/20.tuple.h tests/utiliti

2008-07-08 Thread Travis Vitek 
 

>Author: elemings
>Date: Tue Jul  8 16:13:36 2008
>New Revision: 675044
>
>URL: http://svn.apache.org/viewvc?rev=675044&view=rev
>Log:
>2008-07-08  Eric Lemings <[EMAIL PROTECTED]>
>
>   STDCXX-958
>   * include/tuple: Second parameter in value move ctor of pair
>   specialization missing rvalue reference.
>   (make_tuple, get, relational operators): Explicitly declare
>   as inline functions.
>   (tie): Implemented.
>   * include/rw/_tuple.h: Fix move semantics in heterogenous move
>   assignment operator.
>   (__rw_ignore): Add assignment operator to ignore all values.
>   * tests/utilities/20.tuple.cnstr.cpp: Added V&V for tuple
>   state and invariants.  Manually inspected proper construction
>   of all test tuples.  Updated/corrected/added tests as necessary.
>   * tests/utilities/20.tuple.creation.cpp: Added simple tie()
>   test.
>   * tests/utilities/20.tuple.h: Minor stylistic changes.
>   * tests/utilities/20.tuple.helpers.cpp: Same.
>
>
>Modified:
>stdcxx/branches/4.3.x/include/rw/_tuple.h
>stdcxx/branches/4.3.x/include/tuple
>stdcxx/branches/4.3.x/tests/utilities/20.tuple.cnstr.cpp
>stdcxx/branches/4.3.x/tests/utilities/20.tuple.creation.cpp
>stdcxx/branches/4.3.x/tests/utilities/20.tuple.h
>stdcxx/branches/4.3.x/tests/utilities/20.tuple.helpers.cpp
>
>Modified: stdcxx/branches/4.3.x/include/rw/_tuple.h
>URL: 
>http://svn.apache.org/viewvc/stdcxx/branches/4.3.x/include/rw/_
>tuple.h?rev=675044&r1=675043&r2=675044&view=diff
>===
>===
>--- stdcxx/branches/4.3.x/include/rw/_tuple.h (original)
>+++ stdcxx/branches/4.3.x/include/rw/_tuple.h Tue Jul  8 16:13:36 2008
>@@ -174,7 +174,14 @@
> };
> 
> 
>-struct __rw_ignore { /* empty */ };
>+struct __rw_ignore
>+{
>+template 
>+inline __rw_ignore const&
>+operator= (const _TypeT& /*unused*/) const {
>+return *this;
>+}
>+};
>

I think the commented out parameter name should be removed. I don't see
this in existing code, and I personally find it a bit distracting.

This can probably be changed to use a void return type, which will
simplify the code further. You only really need the return type to chain
assignments or to call a function on the result, none of which we should
be doing.

>Modified: stdcxx/branches/4.3.x/include/tuple
>URL: 
>http://svn.apache.org/viewvc/stdcxx/branches/4.3.x/include/tupl
>e?rev=675044&r1=675043&r2=675044&view=diff
>===
>===
>--- stdcxx/branches/4.3.x/include/tuple (original)
>+++ stdcxx/branches/4.3.x/include/tuple Tue Jul  8 16:13:36 2008
>@@ -211,7 +211,7 @@
> #if !defined _RWSTD_NO_RVALUE_REFERENCES
> 
> template 
>-_EXPLICIT tuple (_TypeU1&& __x, _TypeU2 __y)
>+_EXPLICIT tuple (_TypeU1&& __x, _TypeU2&& __y)
> : _Base (_RWSTD_FORWARD (_TypeU1, __x),
>  _RWSTD_FORWARD (_TypeU2, __y)) { /* empty */ }

Same here with the comment describing the empty function body.

>@@ -377,7 +381,7 @@
> // 20.3.1.5, element access:
> 
> template <_RWSTD_SIZE_T _Index, class _Head, class... _Tail>
>-_TYPENAME tuple_element<_Index, tuple<_Head, _Tail...> >::_Ref
>+inline _TYPENAME tuple_element<_Index, tuple<_Head, _Tail...> >::_Ref
> get (tuple<_Head, _Tail...>& __tuple)
> {
> typedef tuple_element<_Index, tuple<_Head, _Tail...> > _Tuple;

In the recent past Martin recommended not using the _TYPENAME macro. It
isn't hurting anything, but it could probably be removed. I know that I
no longer use it in the traits code. I also noticed the _EXPLICIT macro
above. I think that one should be added to the list. Martin?

>@@ -385,7 +389,7 @@
> }
> 
> template <_RWSTD_SIZE_T _Index, class _Head, class... _Tail>
>-_TYPENAME tuple_element<_Index, tuple<_Head, _Tail...> >::_ConstRef
>+inline _TYPENAME tuple_element<_Index, tuple<_Head, _Tail...> 
>>::_ConstRef
> get (const tuple<_Head, _Tail...>& __tuple)
> {
> typedef tuple_element<_Index, tuple<_Head, _Tail...> > _Tuple;
>@@ -396,59 +400,67 @@
> // 20.3.1.6, relational operators:
> 
> template 
>-bool operator== (const tuple<_TypesT...>& __x,
>- const tuple<_TypesU...>& __y)
>+inline bool
>+operator== (const tuple<_TypesT...>& __x,
>+const tuple<_TypesU...>& __y)
> {
> return _RWSTD_STATIC_CAST (const 
>_RW::__rw_tuple<_TypesT...>&, __x)
>== _RWSTD_STATIC_CAST (const 
>_RW::__rw_tuple<_TypesU...>&, __y);

I think there is a formatting issue here. The prevailing style is for
the operator to start of the next line, but for the operands to be lined
up on their left. As an example

  return__some_really_long_expression_1
 == __some_really_long_expression_2;

I don't really like it all that much, but I'm using it in the traits
code all over the place.

> }
> 
> _RWSTD_SPECIALIZED_FUNCTION
>-bool operator== (const tuple<>& /*__x*/,
>- c