Re: ObjectPtrT::operator-> const problem

On Nov 7, 2007 5:10 PM, Curt Arnold <carnold@xxxxxxxxxx> wrote:

On Nov 7, 2007, at 3:39 PM, Scott McCaskill wrote:

> I've noticed the const version of this method is returning a
> pointer to const.  The reason this is a problem is that it makes it
> impossible to log from const methods when LoggerPtr is a member
> variable:
>
> class C
> {
>     public:
>         C() : logger(log4cxx::Logger::getLogger("my logger")) { }
>
>         void constMethod() const
>         {
>             // Error: can't do this because the const version of
> ObjectPtrT::operator->()
>             // must be used, and it returns a pointer to const T.
>             LOG4CXX_DEBUG(logger, "debug message");
>         }
>
>     private:
>         log4cxx::LoggerPtr logger;
> };
>
> In both versions of operator->, the ObjectPtrT itself cannot be
> modified, so it's not clear why there is a need for a non-const
> version, or for a version that returns a pointer to const.  It
> seems that in this case, the notion of pointer constness is being
> conflated with the notion of pointee constness.
>
> Surely the above example is not such an unreasonable a thing to
> expect to be able to do? :)
>
> BTW, I wasn't sure whether this was more appropriate for the user
> list or the dev list (I'm subscribed to both).
>
> Scott McCaskill
>

log4cxx-dev is likely the more appropriate since things will quickly
delve into minutiae that most users won't care to follow, but I think
most people who could contribute subscribe to both.

I was around that code about a week ago and had the same reaction.  I
did a naive quick fix on my local copy and everything fell apart, so
I quickly reverted that change and moved on to other things.  I
believe that const logging methods has been discussed before either
on the list or in a bug report and might be something to research to
see if any earlier discussion is helpful.

My quick gut reaction is:

a) your analysis is likely right

b) fixing it would likely require creating an alternative template
that takes a pointer parameter, something like:

typedef log4cxx::helpers::ObjectPtrT2<const Logger*> ConstLoggerPtr;
typedef log4cxx::helpers::ObjectPtrT2<Logger*> LoggerPtr;

I wouldn't call it ObjectPtrT2, but I thought that was clearer for
this discussion.

Changing the parameterization parameter for the existing template
would have too much affect on client code.

c) Copying between pointer classes (for example, LoggerPtr,
ConstLoggerPtr and ObjectPtrT<Logger>) would need to be supported as
appropriate.

d) Methods that don't (obviously) change the state (logger->info()
etc) would benefit from being const'd.

If anyone wants to play with it feel free and report back to the list.

Actually, I think there is a simpler solution.  Here's what I tried:

===================================================================
--- src/main/include/log4cxx/helpers/objectptr.h        (revision 593214)
+++ src/main/include/log4cxx/helpers/objectptr.h        (working copy)
@@ -137,8 +137,7 @@
             bool operator!=(const ObjectPtrT& p1) const { return (this->p != p1.p); }
             bool operator==(const T* p1) const { return (this->p == p1); }
             bool operator!=(const T* p1) const { return (this->p != p1); }
-            T* operator->() {return (T*) p; }
-            const T* operator->() const {return (const T*) p; }
+            T* operator->() const {return (T*) p; }
             T& operator*() const {return (T&) *p; }
             operator T*() const {return (T*) p; }


Note that the single version of operator-> that I added can be used anywhere that either of the previous versions could be used, as well as in the test case I originally described.

I tried this with msvc8.  The trunk (593214) builds fine, as do the unit tests.  The unit tests run as well as they did before.  There is a crash midway through, but it crashes in the same place without any local changes.  Naturally, all of my code is also fine with this change.