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.
