ObjectPtrT comments

[Jonathan Wakely <jo...@gmail.com>]

Hi log4cxx developers,

now that I can build the library (thanks for the fix) I've been poking
around and exploring the code.

 ObjectPtrT& operator=(const ObjectPtrT& p1) {
     T* newPtr = (T*) p1.p;
     if (newPtr != 0) {
         newPtr->addRef();
     }
     T** pp = &p;
     void* oldPtr = ObjectPtrBase::exchange((void**) pp, newPtr);
     if (oldPtr != 0) {
         ((T*) oldPtr)->releaseRef();
     }
    return *this;
 }

wow ... that's ugly  :)

What's the point of the C-style sledgehammer cast on the first line of
this function?
How can p1.p not be convertible to T*?  Casts like that do nothing
except hide errors if one day the types do change.

Is there a good reason why ObjectPtrBase::exchange isn't a template,
so it doesn't have to rely on more sledgehammer casts to/from void* ?
I guess that would mean the implementation had to be in the header, so
assuming that's undesirable, I would suggest an
ObjectPtrT<T>::exchange(T**, T*) which does the casts and calls the
base implementation, so that the casts are only needed in one place.

These functions don't need their casts either:
            T* operator->() const {return (T*) p; }
            T& operator*() const {return (T&) *p; }
            operator T*() const {return (T*) p; }
(The last of these functions is a bad idea IMHO, as with most implicit
conversion operators. An explicit ObjectPtrT::get() function would be
better, but that would be an API change, so nevermind.)

This overloaded assignment operator is not const-correct:
     ObjectPtrT& ObjectPtrT::operator=(const T* p1);
This allows:
    typedef const Foo CFoo;
    CFoo* p = new CFoo(some_foo);
    ObjectPtrT<Foo> op;
    op = p;
    *op = another_foo;
    assert( *p == some_foo );   // boom!

If there are cases where the const-ness needs to be removed, it should
be done explicitly by the user of ObjectPtrT, since only the user
knows if a particular case is safe or not.  Doing it automatically
inside ObjectPtrT hides potentially serious errors.
I removed that assignment operator and successfully built and tested
the lib with make check, so I conclude it's not needed anyway.

If you do need to store a pointer-to-const in an ObjectPtrT you can do:
    ObjectPtrT<const Foo> op;
    op = p;

Finally, both overloads of ObjectPtrT::operator< compare pointers
using < which is not guaranteed to be portable by the C++ standard,
see 20.3.3 paragraph 8:
  "For templates greater, less, greater_equal, and less_equal, the
specializations for any pointer type yield a total order, even if the
built-in operators <, >, <=, >= do not."
Therefore for portability std::less should be used to compare pointers.

The attached patch addresses all these points, could it be considered
for inclusion?

It seems from its implementation that ObjectPtrBase::exchange() is not
atomic on 64-bit UNIX platforms, are there any plans to change that?

Jonathan

Re: ObjectPtrT comments

[Jonathan Wakely <jo...@gmail.com>]

On 09/02/2008, Curt Arnold <ca...@apache.org> wrote:
> > These functions don't need their casts either:
> >            T* operator->() const {return (T*) p; }
> >            T& operator*() const {return (T&) *p; }
> >            operator T*() const {return (T*) p; }
> > (The last of these functions is a bad idea IMHO, as with most implicit
> > conversion operators. An explicit ObjectPtrT::get() function would be
> > better, but that would be an API change, so nevermind.)
>
> Well if there is an API change, now is the time to do it.  That is a
> hold over from log4cxx 0.9.7, can't immediately predict the
> implications.  I assume the intended use is to allow use of an
> ObjectPtrT<T> when a function takes a T*.

One danger of an implicit conversion that is specific to smart
pointers is that the conversion also allows:

  ObjectPtrT<Foo> p(new Foo);
  delete p;   // oops!

Jonathan

Re: ObjectPtrT comments

[Jonathan Wakely <jo...@gmail.com>]

On 09/02/2008, Curt Arnold <ca...@apache.org> wrote:
>
> Had a recent issue with const on accessors: https://issues.apache.org/jira/browse/LOGCXX-202

That looks unrelated (except for being on the same line of code).
In the context of those const member functions the type of p is "const
pointer to non-const T" so there is no cast needed to return a
pointer/reference to non-const T.
Constness is shallow in this respect (and since smart pointers model
builtin pointer semantics, shallow constness is what you want - with
e.g. containers it's not.)

> I did play a bit a few weeks ago trying to make things more correct,
> but I started have things start unraveling and I decided spending a
> substantial amount of time trying to repair code that hasn't had any
> reported issues was not the best use of my time.

I spotted a few other non-const-correct areas, I'll try to review them tomorrow.

> The operator< is required for DLL exporting STL collections of
> ObjectPtr's with some version of Microsoft VC (believe it was VC6).
> It should never be called in practice, but is required to be present
> to successfully link.

OK, then it shouldn't matter (comparing pointers with < works on all
common platforms, it's just not guaranteed to on all architectures)

> > The attached patch addresses all these points, could it be considered
> > for inclusion?
>
> I'd definitely separate these concerns so they could be independently
> rolled back if there were unforeseen complications.  Would be great if
> you could file bugs for each of the concerns (http://issues.apache.org/jira
> ).  Something like
>
> LOGCXX-nnn: Unnecessary casts in ObjectPtrT
> LOGCXX-nnn:  Add ObjectPtrT::exchange
> ...

Sure, I'll do that now.

> > It seems from its implementation that ObjectPtrBase::exchange() is not
> > atomic on 64-bit UNIX platforms, are there any plans to change that?
> >
>
>
> APR introduced a apr_xchgptr function at my request, however I don't
> think it has made it into a released version of APR.  If you want to
> propose changes to configure.in that could detect if the APR version
> had apr_xchgptr, I'd be happy to have that as a conditional in the
> implementation of ObjectPtrBase::exchange.

Thanks for the pointer - I'll look into it.

Jonathan

Re: ObjectPtrT comments

[Curt Arnold <ca...@apache.org>]

On Feb 9, 2008, at 4:03 PM, Jonathan Wakely wrote:

> Hi log4cxx developers,
>
> now that I can build the library (thanks for the fix) I've been poking
> around and exploring the code.
>
> ObjectPtrT& operator=(const ObjectPtrT& p1) {
>     T* newPtr = (T*) p1.p;
>     if (newPtr != 0) {
>         newPtr->addRef();
>     }
>     T** pp = &p;
>     void* oldPtr = ObjectPtrBase::exchange((void**) pp, newPtr);
>     if (oldPtr != 0) {
>         ((T*) oldPtr)->releaseRef();
>     }
>    return *this;
> }
>
> wow ... that's ugly  :)
>
> What's the point of the C-style sledgehammer cast on the first line of
> this function?
> How can p1.p not be convertible to T*?  Casts like that do nothing
> except hide errors if one day the types do change.

May have been left over from an previous iteration where p was a  
member of ObjectPtrBase.


>
>
> Is there a good reason why ObjectPtrBase::exchange isn't a template,
> so it doesn't have to rely on more sledgehammer casts to/from void* ?

Yes.  If exchange is going to support atomic operations, it requires  
either platform specific code or APR calls, neither of which should be  
exposed in the header file.  Clients that compile against log4cxx do  
not need APR in their include paths.

>
> I guess that would mean the implementation had to be in the header, so
> assuming that's undesirable, I would suggest an
> ObjectPtrT<T>::exchange(T**, T*) which does the casts and calls the
> base implementation, so that the casts are only needed in one place.

Reasonable.

>
>
> These functions don't need their casts either:
>            T* operator->() const {return (T*) p; }
>            T& operator*() const {return (T&) *p; }
>            operator T*() const {return (T*) p; }
> (The last of these functions is a bad idea IMHO, as with most implicit
> conversion operators. An explicit ObjectPtrT::get() function would be
> better, but that would be an API change, so nevermind.)

Well if there is an API change, now is the time to do it.  That is a  
hold over from log4cxx 0.9.7, can't immediately predict the  
implications.  I assume the intended use is to allow use of an  
ObjectPtrT<T> when a function takes a T*.


>
>
> This overloaded assignment operator is not const-correct:
>     ObjectPtrT& ObjectPtrT::operator=(const T* p1);
> This allows:
>    typedef const Foo CFoo;
>    CFoo* p = new CFoo(some_foo);
>    ObjectPtrT<Foo> op;
>    op = p;
>    *op = another_foo;
>    assert( *p == some_foo );   // boom!
>
> If there are cases where the const-ness needs to be removed, it should
> be done explicitly by the user of ObjectPtrT, since only the user
> knows if a particular case is safe or not.  Doing it automatically
> inside ObjectPtrT hides potentially serious errors.
> I removed that assignment operator and successfully built and tested
> the lib with make check, so I conclude it's not needed anyway.


Had a recent issue with const on accessors: https://issues.apache.org/jira/browse/LOGCXX-202 
.

I did play a bit a few weeks ago trying to make things more correct,  
but I started have things start unraveling and I decided spending a  
substantial amount of time trying to repair code that hasn't had any  
reported issues was not the best use of my time.

>
>
> If you do need to store a pointer-to-const in an ObjectPtrT you can  
> do:
>    ObjectPtrT<const Foo> op;
>    op = p;
>
> Finally, both overloads of ObjectPtrT::operator< compare pointers
> using < which is not guaranteed to be portable by the C++ standard,
> see 20.3.3 paragraph 8:
>  "For templates greater, less, greater_equal, and less_equal, the
> specializations for any pointer type yield a total order, even if the
> built-in operators <, >, <=, >= do not."
> Therefore for portability std::less should be used to compare  
> pointers.
>


The operator< is required for DLL exporting STL collections of  
ObjectPtr's with some version of Microsoft VC (believe it was VC6).   
It should never be called in practice, but is required to be present  
to successfully link.



> The attached patch addresses all these points, could it be considered
> for inclusion?

I'd definitely separate these concerns so they could be independently  
rolled back if there were unforeseen complications.  Would be great if  
you could file bugs for each of the concerns (http://issues.apache.org/jira 
).  Something like

LOGCXX-nnn: Unnecessary casts in ObjectPtrT
LOGCXX-nnn:  Add ObjectPtrT::exchange
...


>
>
> It seems from its implementation that ObjectPtrBase::exchange() is not
> atomic on 64-bit UNIX platforms, are there any plans to change that?
>


APR introduced a apr_xchgptr function at my request, however I don't  
think it has made it into a released version of APR.  If you want to  
propose changes to configure.in that could detect if the APR version  
had apr_xchgptr, I'd be happy to have that as a conditional in the  
implementation of ObjectPtrBase::exchange.