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
Index: src/main/include/log4cxx/helpers/objectptr.h
===================================================================
--- src/main/include/log4cxx/helpers/objectptr.h (revision 620195)
+++ src/main/include/log4cxx/helpers/objectptr.h (working copy)
@@ -19,6 +19,7 @@
#define _LOG4CXX_HELPERS_OBJECT_PTR_H
#include <log4cxx/log4cxx.h>
+#include <functional>
//
// Helgrind (race detection tool for Valgrind) will complain if pointer
@@ -44,7 +45,7 @@
ObjectPtrBase();
virtual ~ObjectPtrBase();
static void checkNull(const int& null);
- static void* exchange(void** destination, void* newValue);
+ static void* exchangeVoidP(void** destination, void* newValue);
virtual void* cast(const Class& cls) const = 0;
};
@@ -109,14 +110,13 @@
}
ObjectPtrT& operator=(const ObjectPtrT& p1) {
- T* newPtr = (T*) p1.p;
+ T* newPtr = p1.p;
if (newPtr != 0) {
newPtr->addRef();
}
- T** pp = &p;
- void* oldPtr = ObjectPtrBase::exchange((void**) pp, newPtr);
+ T* oldPtr = ObjectPtrT::exchange(&p, newPtr);
if (oldPtr != 0) {
- ((T*) oldPtr)->releaseRef();
+ oldPtr->releaseRef();
}
return *this;
}
@@ -127,10 +127,9 @@
// throws IllegalArgumentException if null != 0
//
ObjectPtrBase::checkNull(null);
- T** pp = &p;
- void* oldPtr = ObjectPtrBase::exchange((void**) pp, 0);
+ T* oldPtr = ObjectPtrT::exchange(&p, (T*)0);
if (oldPtr != 0) {
- ((T*) oldPtr)->releaseRef();
+ oldPtr->releaseRef();
}
return *this;
}
@@ -139,26 +138,13 @@
if (p1 != 0) {
p1->addRef();
}
- T** pp = &p;
- void* oldPtr = ObjectPtrBase::exchange((void**) pp, p1);
+ T* oldPtr = ObjectPtrT::exchange(&p, p1);
if (oldPtr != 0) {
- ((T*)oldPtr)->releaseRef();
+ oldPtr->releaseRef();
}
return *this;
}
- ObjectPtrT& operator=(const T* p1) {
- if (p1 != 0) {
- p1->addRef();
- }
- T** pp = &p;
- void* oldPtr = ObjectPtrBase::exchange((void**) pp, const_cast<T*>(p1));
- if (oldPtr != 0) {
- ((T*)oldPtr)->releaseRef();
- }
- return *this;
- }
-
ObjectPtrT& operator=(ObjectPtrBase& p1) {
T* newPtr = reinterpret_cast<T*>(p1.cast(T::getStaticClass()));
return operator=(newPtr);
@@ -171,17 +157,22 @@
bool operator==(const ObjectPtrT& p1) const { return (this->p == p1.p); }
bool operator!=(const ObjectPtrT& p1) const { return (this->p != p1.p); }
- bool operator<(const ObjectPtrT& p1) const { return (this->p < p1.p); }
+ bool operator<(const ObjectPtrT& p1) const { return std::less<const T*>(this->p, p1.p); }
bool operator==(const T* p1) const { return (this->p == p1); }
bool operator!=(const T* p1) const { return (this->p != p1); }
- bool operator<(const T* p1) const { return (this->p < p1); }
- T* operator->() const {return (T*) p; }
- T& operator*() const {return (T&) *p; }
- operator T*() const {return (T*) p; }
+ bool operator<(const T* p1) const { return std::less<const T*>(this->p, p1); }
+ T* operator->() const { return p; }
+ T& operator*() const { return *p; }
+ operator T*() const { return p; }
private:
+ static T* exchange(T** ptr, T* val) {
+ return static_cast<T*>(
+ ObjectPtrBase::exchangeVoidP((void**)ptr, val) );
+ }
+
T * p;
virtual void* cast(const Class& cls) const {
if (p != 0) {
@@ -192,7 +183,6 @@
};
-
}
}
Index: src/main/cpp/objectptr.cpp
===================================================================
--- src/main/cpp/objectptr.cpp (revision 620195)
+++ src/main/cpp/objectptr.cpp (working copy)
@@ -33,7 +33,7 @@
}
}
-void* ObjectPtrBase::exchange(void** destination, void* newValue) {
+void* ObjectPtrBase::exchangeVoidP(void** destination, void* newValue) {
#if _WIN32 && (!defined(_MSC_VER) || _MSC_VER >= 1300)
return InterlockedExchangePointer(destination, newValue);
#elif APR_SIZEOF_VOIDP == 4
