Re: svn commit: r678913 - in /stdcxx/branches/4.3.x: ./ etc/config/src/ examples/include/ include/ include/loc/ include/rw/ src/ tests/containers/ tests/localization/ tests/strings/ tests/utilities/

[Martin Sebor]

elemings@... wrote:
> Author: elemings
> Date: Tue Jul 22 14:24:01 2008
> New Revision: 678913
>
> URL: http://svn.apache.org/viewvc?rev=678913&view=rev
[...]

> Modified: stdcxx/branches/4.3.x/include/deque
> URL: http://svn.apache.org/viewvc/stdcxx/branches/4.3.x/include/deque?rev=678913&r1=678912&r2=678913&view=diff
> ==============================================================================
> --- stdcxx/branches/4.3.x/include/deque (original)
> +++ stdcxx/branches/4.3.x/include/deque Tue Jul 22 14:24:01 2008
> @@ -58,8 +58,6 @@
>  template <class _TypeT, class _Allocator = allocator<_TypeT> >
>  class deque;
>  
> -#ifdef _RWSTD_NO_MEMBER_TEMPLATES
> -

I think we want to get rid of this whole block, no?

>  // declarations of non-member function templates implementing
>  // the functionality of deque member function templates
>  
[...]

> Modified: stdcxx/branches/4.3.x/include/deque.cc
> URL: http://svn.apache.org/viewvc/stdcxx/branches/4.3.x/include/deque.cc?rev=678913&r1=678912&r2=678913&view=diff
> ==============================================================================
> --- stdcxx/branches/4.3.x/include/deque.cc (original)
> +++ stdcxx/branches/4.3.x/include/deque.cc Tue Jul 22 14:24:01 2008
> @@ -518,8 +518,6 @@
>  }
>  
>  
> -#ifndef _RWSTD_NO_MEMBER_TEMPLATES
> -
>  template <class _TypeT, class _Allocator>
>  template <class _InputIter>
>  void deque<_TypeT, _Allocator>::
> @@ -529,18 +527,6 @@
>  
>      deque* const __self = this;

And here we should be able to do away with the __self hack. This
hack, btw., is probably used in other containers (string comes
to mind, although it doesn't look to me like your patch removes
this cruft from string), so we should review and clean those up
as well. Otherwise these strange looking vestiges will leave
people wondering what the heck we're doing.


>  
[...]

> Modified: stdcxx/branches/4.3.x/include/vector
> URL: http://svn.apache.org/viewvc/stdcxx/branches/4.3.x/include/vector?rev=678913&r1=678912&r2=678913&view=diff
> ==============================================================================
> --- stdcxx/branches/4.3.x/include/vector (original)
> +++ stdcxx/branches/4.3.x/include/vector Tue Jul 22 14:24:01 2008
> @@ -58,8 +58,6 @@
>  template <class _TypeT, class _Allocator = allocator<_TypeT> >
>  class vector;
>  
> -#ifdef _RWSTD_NO_MEMBER_TEMPLATES
> -

Same as in deque: this is #ifdef, not #ifndef, so shouldn't we
delete the whole block?

[...]

> @@ -1342,8 +1329,11 @@
>          __x=_x;
>        }
>      }
> +
>      static void swap(reference __x, reference __y);
> +
>      void flip ();
> +
>      void clear()

These are good changes but in the future please resist the urge
to improve formatting in the same patch as where you're making
substantive changes.

>      {
>        erase(begin(),end());
>
> Modified: stdcxx/branches/4.3.x/include/vector.cc
> URL: http://svn.apache.org/viewvc/stdcxx/branches/4.3.x/include/vector.cc?rev=678913&r1=678912&r2=678913&view=diff
> ==============================================================================
> --- stdcxx/branches/4.3.x/include/vector.cc (original)
> +++ stdcxx/branches/4.3.x/include/vector.cc Tue Jul 22 14:24:01 2008
[...]
> @@ -393,23 +378,6 @@
>  {
>      vector* const __self = this;


Here's the __self hack again.

[...]
> @@ -487,21 +453,6 @@
>  {
>      vector* const __self = this;

And again.

>  
[...]
> @@ -588,25 +537,6 @@
>  {
>      vector* const __self = this;

And again.

>  
> Modified: stdcxx/branches/4.3.x/src/vecbool.cpp
> URL: http://svn.apache.org/viewvc/stdcxx/branches/4.3.x/src/vecbool.cpp?rev=678913&r1=678912&r2=678913&view=diff
> ==============================================================================
> --- stdcxx/branches/4.3.x/src/vecbool.cpp (original)
> +++ stdcxx/branches/4.3.x/src/vecbool.cpp Tue Jul 22 14:24:01 2008

FYI: we'll be able to remove this whole file once we do the same
cleanup for partial specialization. We should open an issue for
it, too.

[...]

> Modified: stdcxx/branches/4.3.x/tests/localization/22.locale.synopsis.cpp
> URL: http://svn.apache.org/viewvc/stdcxx/branches/4.3.x/tests/localization/22.locale.synopsis.cpp?rev=678913&r1=678912&r2=678913&view=diff
> ==============================================================================
> --- stdcxx/branches/4.3.x/tests/localization/22.locale.synopsis.cpp (original)
> +++ stdcxx/branches/4.3.x/tests/localization/22.locale.synopsis.cpp Tue Jul 22 14:24:01 2008
> @@ -442,13 +442,9 @@
>  
>      static std::locale::id id;
>  
> -#ifdef _RWSTD_NO_MEMBER_TEMPLATES
> -

Same as in <deque> and <vector>: I think we want to delete the whole
block both here...

>      virtual std::locale::id& _C_get_id () const {
>          return id;
>      }
> -
> -#endif   // _RWSTD_NO_MEMBER_TEMPLATES
>  };
>  
>  
> @@ -571,14 +567,12 @@
>              ~Facet () {
>                  is_dtor_virtual = true;
>              }
> -#ifdef _RWSTD_NO_MEMBER_TEMPLATES

...and here.

>  
>              virtual std::locale::id& _C_get_id () const {
>                  static std::locale::id id;
>                  return id;
>              }
>  
> -#endif   // _RWSTD_NO_MEMBER_TEMPLATES

Martin

[Eric Lemings-2]

 

> -----Original Message-----
> From: Martin Sebor [mailto:msebor@...] On Behalf Of Martin Sebor
> Sent: Tuesday, July 22, 2008 4:31 PM
> To: dev@...
> Subject: Re: svn commit: r678913 - in /stdcxx/branches/4.3.x:
> ./ etc/config/src/ examples/include/ include/ include/loc/
> include/rw/ src/ tests/containers/ tests/localization/
> tests/strings/ tests/utilities/
>
...

> ==============================================================
> ================
> > --- stdcxx/branches/4.3.x/include/deque.cc (original)
> > +++ stdcxx/branches/4.3.x/include/deque.cc Tue Jul 22 14:24:01 2008
> > @@ -518,8 +518,6 @@
> >  }
> >  
> >  
> > -#ifndef _RWSTD_NO_MEMBER_TEMPLATES
> > -
> >  template <class _TypeT, class _Allocator>
> >  template <class _InputIter>
> >  void deque<_TypeT, _Allocator>::
> > @@ -529,18 +527,6 @@
> >  
> >      deque* const __self = this;
>
> And here we should be able to do away with the __self hack. This
> hack, btw., is probably used in other containers (string comes
> to mind, although it doesn't look to me like your patch removes
> this cruft from string), so we should review and clean those up
> as well. Otherwise these strange looking vestiges will leave
> people wondering what the heck we're doing.

Since this change is not related to STDCXX-978 and involves other
containers not affected by this issue, we should create a new
issue and do this cleanup as part of that issue, yes?

>
...

> > +
> >      static void swap(reference __x, reference __y);
> > +
> >      void flip ();
> > +
> >      void clear()
>
> These are good changes but in the future please resist the urge
> to improve formatting in the same patch as where you're making
> substantive changes.

I agree in principle but for very small changes like this I probably
wouldn't bother going to the trouble of doing this separately.  :)

I'm incorporating all other recommendations.

Thanks,
Brad.

[Martin Sebor]

Eric Lemings wrote:

>  
>
>> -----Original Message-----
>> From: Martin Sebor [mailto:msebor@...] On Behalf Of Martin Sebor
>> Sent: Tuesday, July 22, 2008 4:31 PM
>> To: dev@...
>> Subject: Re: svn commit: r678913 - in /stdcxx/branches/4.3.x:
>> ./ etc/config/src/ examples/include/ include/ include/loc/
>> include/rw/ src/ tests/containers/ tests/localization/
>> tests/strings/ tests/utilities/
>>
> ...
>> ==============================================================
>> ================
>>> --- stdcxx/branches/4.3.x/include/deque.cc (original)
>>> +++ stdcxx/branches/4.3.x/include/deque.cc Tue Jul 22 14:24:01 2008
>>> @@ -518,8 +518,6 @@
>>>  }
>>>  
>>>  
>>> -#ifndef _RWSTD_NO_MEMBER_TEMPLATES
>>> -
>>>  template <class _TypeT, class _Allocator>
>>>  template <class _InputIter>
>>>  void deque<_TypeT, _Allocator>::
>>> @@ -529,18 +527,6 @@
>>>  
>>>      deque* const __self = this;
>> And here we should be able to do away with the __self hack. This
>> hack, btw., is probably used in other containers (string comes
>> to mind, although it doesn't look to me like your patch removes
>> this cruft from string), so we should review and clean those up
>> as well. Otherwise these strange looking vestiges will leave
>> people wondering what the heck we're doing.
>
> Since this change is not related to STDCXX-978 and involves other
> containers not affected by this issue, we should create a new
> issue and do this cleanup as part of that issue, yes?

I believe it is directly related. The hack is part of the
workaround for _RWSTD_NO_MEMBER_TEMPLATES (there shouldn't
be any other places where we use it).

>
> ...
>>> +
>>>      static void swap(reference __x, reference __y);
>>> +
>>>      void flip ();
>>> +
>>>      void clear()
>> These are good changes but in the future please resist the urge
>> to improve formatting in the same patch as where you're making
>> substantive changes.
>
> I agree in principle but for very small changes like this I probably
> wouldn't bother going to the trouble of doing this separately.  :)

There are good reasons to keep formatting changes out of
unrelated patches. Besides those mentioned on our page
below, it makes reviewing big patches much easier and more
reliable.

   http://stdcxx.apache.org/bugs.html#patch_format

Thanks
Martin

>
> I'm incorporating all other recommendations.
>
> Thanks,
> Brad.