You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stdcxx.apache.org by Farid Zaripov <Fa...@kyiv.vdiweb.com> on 2007/02/07 16:53:38 UTC
[PATCH] Fix of STDCXX-268, STDCXX-331
Attached is a proposed patch for fix the bugs STDCXX-268 and STDCXX-331.
Actually this patch fixes the bug STDCXX-331, but since the std::list
constructor uses insert() method so the bug STDCXX-268 is fixed
automatically.
ChangeLog:
STDCXX-268
STDCXX-331
* list (_RWSTD_LIST_SAFE_INSERT_RANGE): New macro for exception
safe inserting the range into the list.
(_C_insert): Used _RWSTD_LIST_SAFE_INSERT_RANGE macro.
(_C_insert): Added try/catch with removing the inserted elements
if exception was thrown.
Farid.
RE: [PATCH] Fix of STDCXX-268, STDCXX-331
Posted by Farid Zaripov <Fa...@kyiv.vdiweb.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Wednesday, February 14, 2007 2:47 AM
> To: stdcxx-dev@incubator.apache.org
> Subject: Re: [PATCH] Fix of STDCXX-268, STDCXX-331
>
> So unless someone objects (I assume you're fine with it) I
> propose we ditch the _RWSTD_NO_INLINE_MEMBER_TEMPLATES config
> macro and all the workarounds for it. We should do a clean
> sweep through the library to remove it rather than removing
> it piecemeal.
>
> We can either remove the macro first and then commit a
> cleaner version of your patch w/o or we can do it the other
> way around.
> Do you have a preference? (If the latter we should probably
> open a Jira task as a reminder.)
Commited thus: http://svn.apache.org/viewvc?view=rev&rev=507971
Created JIRA task: http://issues.apache.org/jira/browse/STDCXX-339
> >> Btw., I wonder if we could simplify (optimize) this code so as to
> >> call erase(__start, __it) in the catch block instead of
> looping (the
> >> idea is that the range form of erase() might be more
> efficient than
> >> calling the single form repeatedly):
> >
> > Yes, but to get __start we need use loop or std::advance, because
> > __start = __it - __n.
> >
> > iterator __start = __it;
> > std::advance (__start, -n);
> > erase (__start, __it);
> >
> > Another approach is to remember the iterator after first
> successful
> > insert (__it, *__first) operation.
> >
> > What do you prefer?
>
> Definitely the latter. Looping would be expensive.
Done.
Farid.
Re: [PATCH] Fix of STDCXX-268, STDCXX-331
Posted by Martin Sebor <se...@roguewave.com>.
Farid Zaripov wrote:
>> -----Original Message-----
>> From: Martin Sebor [mailto:sebor@roguewave.com]
>> Sent: Thursday, February 08, 2007 2:36 AM
>> To: stdcxx-dev@incubator.apache.org
>> Subject: Re: [PATCH] Fix of STDCXX-268, STDCXX-331
>>
>> Farid Zaripov wrote:
>>> Attached is a proposed patch for fix the bugs STDCXX-268
>> and STDCXX-331.
>>
>> If the bodies of the functions are the same wouldn't be
>> better to call one from the other rather than introducing the macro?
>> (The macro makes it impossible to step through the functions
>> in most debuggers. I know we have macros in there already but
>> we don't need to make things worse by adding more :)
> I agree that it would be better.
>
>> Or are we invoking the macro with arguments of different
>> types each time (Iterator, pointer, const_pointer) because
>> we're working around the lack of member templates?
> Yes. If _RWSTD_NO_INLINE_MEMBER_TEMPLATES macro
> is defined then the range form of the method insert is present twice
> with types const_pointer and const_iterator.
>
>> (We should discuss and decide if we want to keep these workarounds or
> if
>> it's time to get rid of them and assume that the compiler is
> reasonably modern).
> I don't have the compiler without supporting the inline mebmer
> templates.
> We need to make the list of the compilers without supporting the inline
> member
> templates and then decide.
I don't know of any C++ compiler that's in widespread use
that doesn't support member templates. Even MSVC 6 handles
them (it just doesn't grok member templates defined outside
the body of the enclosing class).
So unless someone objects (I assume you're fine with it) I
propose we ditch the _RWSTD_NO_INLINE_MEMBER_TEMPLATES config
macro and all the workarounds for it. We should do a clean
sweep through the library to remove it rather than removing
it piecemeal.
We can either remove the macro first and then commit a cleaner
version of your patch w/o or we can do it the other way around.
Do you have a preference? (If the latter we should probably
open a Jira task as a reminder.)
>
>> Btw., I wonder if we could simplify (optimize) this code so
>> as to call erase(__start, __it) in the catch block instead of
>> looping (the idea is that the range form of erase() might be
>> more efficient than calling the single form repeatedly):
>
> Yes, but to get __start we need use loop or std::advance, because
> __start = __it - __n.
>
> iterator __start = __it;
> std::advance (__start, -n);
> erase (__start, __it);
>
> Another approach is to remember the iterator after first successful
> insert (__it, *__first)
> operation.
>
> What do you prefer?
Definitely the latter. Looping would be expensive.
Martin
RE: [PATCH] Fix of STDCXX-268, STDCXX-331
Posted by Farid Zaripov <Fa...@kyiv.vdiweb.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Thursday, February 08, 2007 2:36 AM
> To: stdcxx-dev@incubator.apache.org
> Subject: Re: [PATCH] Fix of STDCXX-268, STDCXX-331
>
> Farid Zaripov wrote:
> > Attached is a proposed patch for fix the bugs STDCXX-268
> and STDCXX-331.
>
> If the bodies of the functions are the same wouldn't be
> better to call one from the other rather than introducing the macro?
> (The macro makes it impossible to step through the functions
> in most debuggers. I know we have macros in there already but
> we don't need to make things worse by adding more :)
I agree that it would be better.
> Or are we invoking the macro with arguments of different
> types each time (Iterator, pointer, const_pointer) because
> we're working around the lack of member templates?
Yes. If _RWSTD_NO_INLINE_MEMBER_TEMPLATES macro
is defined then the range form of the method insert is present twice
with types const_pointer and const_iterator.
> (We should discuss and decide if we want to keep these workarounds or
if
> it's time to get rid of them and assume that the compiler is
reasonably modern).
I don't have the compiler without supporting the inline mebmer
templates.
We need to make the list of the compilers without supporting the inline
member
templates and then decide.
> Btw., I wonder if we could simplify (optimize) this code so
> as to call erase(__start, __it) in the catch block instead of
> looping (the idea is that the range form of erase() might be
> more efficient than calling the single form repeatedly):
Yes, but to get __start we need use loop or std::advance, because
__start = __it - __n.
iterator __start = __it;
std::advance (__start, -n);
erase (__start, __it);
Another approach is to remember the iterator after first successful
insert (__it, *__first)
operation.
What do you prefer?
Farid.
Re: [PATCH] Fix of STDCXX-268, STDCXX-331
Posted by Martin Sebor <se...@roguewave.com>.
Farid Zaripov wrote:
> Attached is a proposed patch for fix the bugs STDCXX-268 and STDCXX-331.
If the bodies of the functions are the same wouldn't be better
to call one from the other rather than introducing the macro?
(The macro makes it impossible to step through the functions
in most debuggers. I know we have macros in there already but
we don't need to make things worse by adding more :)
Or are we invoking the macro with arguments of different types
each time (Iterator, pointer, const_pointer) because we're
working around the lack of member templates? (We should
discuss and decide if we want to keep these workarounds or
if it's time to get rid of them and assume that the compiler
is reasonably modern).
Btw., I wonder if we could simplify (optimize) this code so
as to call erase(__start, __it) in the catch block instead
of looping (the idea is that the range form of erase() might
be more efficient than calling the single form repeatedly):
+# define _RWSTD_LIST_SAFE_INSERT_RANGE(__it, __first, __last) \
+ _RWSTD_ASSERT_RANGE (begin (), __it); \
+ _RWSTD_ASSERT_RANGE (__first, __last); \
+ size_type __n = 0; \
+ _TRY { \
+ for ( ; !(__first == __last); ++__first, ++__n) \
+ insert (__it, *__first); \
+ } _CATCH (...) { \
+ for ( ; __n; --__n) { \
+ iterator __prev = __it; \
+ erase (--__prev); \
+ } \
+ _RETHROW; \
+ } typedef void dummy_t
+
+
[Oh, also, be careful to privatize all names: dummy_t should
be __dummy_t (or __rw_dummy or something like that).]
Martin
>
> Actually this patch fixes the bug STDCXX-331, but since the std::list
> constructor uses insert() method so the bug STDCXX-268 is fixed
> automatically.
>
> ChangeLog:
> STDCXX-268
> STDCXX-331
> * list (_RWSTD_LIST_SAFE_INSERT_RANGE): New macro for exception
> safe inserting the range into the list.
> (_C_insert): Used _RWSTD_LIST_SAFE_INSERT_RANGE macro.
> (_C_insert): Added try/catch with removing the inserted elements
> if exception was thrown.
>
> Farid.
>