WorldForge

#ifndef UTIL_LOGGING_H
#define UTIL_LOGGING_H

// STAGE
// Copyright (C) 2000 Bryce Harrington and The WorldForge Project
//
//  This program is free software; you can redistribute it and/or modify

//  it under the terms of the GNU General Public License as published by

//  the Free Software Foundation; either version 2 of the License, or
//  (at your option) any later version.
//
//  This program is distributed in the hope that it will be useful,
//  but WITHOUT ANY WARRANTY; without even the implied warranty of
//  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
//  GNU General Public License for more details.
//
//  You should have received a copy of the GNU General Public License
//  along with this program; if not, write to the Free Software
//  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
//
//  For information about Worldforge and its authors, please contact
//  the Worldforge Web Site at http://www.wordforge.org.

//  Author: Tommi Laukkanen (Morgenes)
//  Created: April 27 2001 by Tommi Laukkanen

// This file declares classes enabling specializable logging
functionality
// of util library.

// $Log: util_logging.h,v $
// Revision 1.2  2001/06/29 20:50:29  tlaukkan
// Util library logging functionality is now in place. See util_thread
for
// example of using it. Galileo has now Logger class which extends
Util::Logger
// and enables util classes to log via galileo in Stage.
//
// Revision 1.1  2001/06/29 19:34:06  tlaukkan
// *** empty log message ***
//

#include <iostream>
#include <string>

namespace Util {

  /**
   * Logger is simple log message dispatcher.
   * It can be specialized for different log systems.
   */
  class Logger {
  public:

    /**
     * Clones Logger instance. Needed to enable virtual cloning.
     * @return The new Logger instance.
     */
    virtual Logger* clone() {
      Logger *logger=new Logger();
      *logger=*this;
      return logger;
    }

    /**
     * Dispatches debug messages.
     * @param m The message string.
     */
    virtual void debug(const string &m) {
      cout << m << '\n';
    }

    /**
     * Dispatches log messages.
     * @param m The message string.
     */
    virtual void log(const string &m) {
      cout << m << '\n';
    }

  };

  /**
   * Loggable is base class for objects using Util logging framework.
   * Each Loggable have reference to Logger instance which does log
   * dispatching.
   */
  class Loggable {
  public:
    /**
     * Default constructor. Creates default Logger for dispatching.
     */
    Loggable() {
      m_logger=new Logger();
    }

    /**
     * Constructor for specialized dispatching.
     * @param logger The specialized logger instance.
     */
    Loggable(Logger *logger) {
      assert(logger);
      m_logger=logger;
    }

    /**
     * Destructor. Destructs the Logger instance.
     */
    virtual ~Loggable() {
      delete m_logger;
    }

    /**
     * Dispatches debug messages.
     * @param name The name of the Loggable instance.
     * @param method The method were log message were invoked.
     * @param m The message string.
     */
    virtual void debug(const string &name, const string &method,const
string &message); // Used for sending debug messages.

    /**
     * Dispatches log messages.
     * @param name The name of the Loggable instance.
     * @param method The method were log message were invoked.
     * @param m The message string.
     */
    virtual void log(const string &name, const string &method,const
string &message); // Used for sending log messages.

  protected:
    Logger *m_logger;
  };

}

#endif


// STAGE
// Copyright (C) 2000 Bryce Harrington and The WorldForge Project
//
//  This program is free software; you can redistribute it and/or modify

//  it under the terms of the GNU General Public License as published by

//  the Free Software Foundation; either version 2 of the License, or
//  (at your option) any later version.
//
//  This program is distributed in the hope that it will be useful,
//  but WITHOUT ANY WARRANTY; without even the implied warranty of
//  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
//  GNU General Public License for more details.
//
//  You should have received a copy of the GNU General Public License
//  along with this program; if not, write to the Free Software
//  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
//
//  For information about Worldforge and its authors, please contact
//  the Worldforge Web Site at http://www.wordforge.org.

//  Author: Tommi Laukkanen (Morgenes)
//  Created: April 27 2001 by Tommi Laukkanen

// This file defines classes enabling specializable logging
functionality
// of util library.

// $Log: util_logging.cpp,v $
// Revision 1.2  2001/06/29 20:50:29  tlaukkan
// Util library logging functionality is now in place. See util_thread
for
// example of using it. Galileo has now Logger class which extends
Util::Logger
// and enables util classes to log via galileo in Stage.
//
// Revision 1.1  2001/06/29 19:34:06  tlaukkan
// *** empty log message ***
//

#include "util_logging.h"

namespace Util {

void Loggable::debug(const string &name, const string &method,const
string &message) {
  string mes=name;
  mes+=" : ";
  mes+=method;
  mes+=" : ";
  mes+=message;
  m_logger->debug(mes);
}

void Loggable::log(const string &name, const string &method,const string
&message) {
  string mes=name;
  mes+=" : ";
  mes+=method;
  mes+=" : ";
  mes+=message;
  m_logger->log(mes);
}

}


_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

I'm not sure I like the design of these classes. I think of logging 
as a system service global to the app. But these classes make logging 
an object level policy which means you can't do things like changing 
the log destinations without touching a gazillion places in the code.

I've never written any server side software, but I imagine logging 
would be pretty important and I don't think the stuff I've seen so 
far is adequate.

>   /**
>    * Logger is simple log message dispatcher.
>    * It can be specialized for different log systems.
>    */

Better to say something like "Logger is the class that does the 
writing to a particular destination. The default writes to 
std::cout". Dispatcher has the connotation of selecting between 
various destinations.

>   class Logger {
>   public:

There should be a virtual dtor and the copy ctor and assignment 
operators should be disabled.

>     /**
>      * Clones Logger instance. Needed to enable virtual cloning.
>      * @return The new Logger instance.
>      */
>     virtual Logger* clone() {
>       Logger *logger=new Logger();
>       *logger=*this;
>       return logger;
>     }

Clone methods should always be const. It's not safe to rely on the 
assignment operator here (if the object is a subclass and they forget 
to overrride clone() the object will be sliced).

>     /**
>      * Dispatches debug messages.
>      * @param m The message string.
>      */

This is a style issue but I really hate comments that convey no 
information (eg labeling a dtor or a default ctor) or can be avoided 
by tweaking the code (using "mesg" instead of "m").

>     virtual void debug(const string &m) {
>       cout << m << '\n';
>     }

It's bad style to inline virtual methods. It's even worse to inline 
iostream calls (this will inject massive amounts of object code into 
every file that uses Logger which the linker will eventually dead 
strip). I'm not sure it's wise to add a new line here: it makes it 
awkward for users who want to write lines out piecemeal.

>     /**
>      * Dispatches log messages.
>      * @param m The message string.
>      */
>     virtual void log(const string &m) {
>       cout << m << '\n';
>     }

Do we really need both debug() and log() methods? Wouldn't it be 
better to wrap the debug logging with #if DEBUG?

>   };
>
>   /**
>    * Loggable is base class for objects using Util logging framework.
>    * Each Loggable have reference to Logger instance which does log
>    * dispatching.
>    */

If you're going to do this you should treat Loggable as a mixin (and 
there should be some agreement on naming and appropriate semantics).

 From your other post you see this as working as follows:

class AIEngine : public Loggable {
public:
    AIEngine(Logger* log) : Loggable(logger) {}

   void think() {this->log("AIEngine", "AIEngine:: think", "Thinking 
deep thoughts"); /* do stuff */}
};

Wouldn't it be better to ditch Loggable, the questionable public 
inheritance, and rely on composition? Something like this:

class AIEngine  {
public:
    ~AIEngine() {delete mLog;}

    AIEngine(Logger* takeLog) : mLog (takeLog) {}

   void think () {mLog->log("AIEngine : Thinking deep thoughts"); /* 
do stuff */}

private:
    Logger* mLog;
};

>   class Loggable {

Base classes should pretty much always disable the copy ctor and the 
assignment operator.

>     /**
>      * Dispatches debug messages.
>      * @param name The name of the Loggable instance.
>      * @param method The method were log message were invoked.
>      * @param m The message string.
>      */
>     virtual void debug(const string &name, const string &method,const
>string &message); // Used for sending debug messages.

It's not at all clear from the comments what these are for. 
Filtering? Annotating the message? Something else? From the code it 
seems to be annotating.

It might be worthwhile putting a flush() method in here. It's 
important to regularly flush things like file sinks so that you don't 
lose information in the event of a crash, but doing it on every line 
might have performance implications.

   -- Jesse
_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

On 03 Jul 2001 11:08:31 -0700, Jesse Jones wrote:

> >   class Logger {
> >   public:
> 
> >     /**
> >      * Dispatches debug messages.
> >      * @param m The message string.
> >      */
> 
> This is a style issue but I really hate comments that convey no 
> information (eg labeling a dtor or a default ctor) or can be avoided 
> by tweaking the code (using "mesg" instead of "m").

Actually this is for the benefit of Doxygen :-)

>  From your other post you see this as working as follows:
> 
> class AIEngine : public Loggable {
> public:
>     AIEngine(Logger* log) : Loggable(logger) {}
> 
>    void think() {this->log("AIEngine", "AIEngine:: think", "Thinking 
> deep thoughts"); /* do stuff */}
> };
> 
> Wouldn't it be better to ditch Loggable, the questionable public 
> inheritance, and rely on composition? Something like this:
> 
> class AIEngine  {
> public:
>     ~AIEngine() {delete mLog;}
> 
>     AIEngine(Logger* takeLog) : mLog (takeLog) {}
> 
>    void think () {mLog->log("AIEngine : Thinking deep thoughts"); /* 
> do stuff */}
> 
> private:
>     Logger* mLog;
> };
> 
> >   class Loggable {

I consider both of these design annoying; my requirements are:
    - logging be globall available; i.e static methods or functions;
    the virtualisation should be hidden.

    - printf style args

So I would like the publci API (obviosuly with a virtual class inside
to do the clever ness) to look like this:

namespace Log {

Log(const string component, const char *format, ...);

Debug( ... ); 

};

Both the inheritance and mixin strategies are needlessly complicated
unless you want multiple logging streams or so; if people want to expose
that, fine; but at a minimum we need something highly compact which will
'just work' as easily as printf().

James

_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

How are these tests invoked? A separate test app? When the server 
starts up? Have you looked at any of the prior art on unit test 
frameworks?

>/**
>  * Tester class is a composite member of Testable class
>  * and acts as interface to Testable class for TestTool.
>  */

This comment only makes sense once you understand how everything fits together.

>class Tester : public Loggable {
>  friend Testable;
>  public:
>  /**
>   * Constructor
>   * @param name The name of the testable instance.
>   * @param testable The mother testable.
>   * @param logger The logger instance for log message dispatching.
>   */
>  Tester(const string &name,Testable *testable,Logger *logger);

Why do Tester and Testable both have names? It seems like 
Tester::name() should call m_testable->name().

>  /**
>   * Method for generic test cases for TestTool.
>   * @return True if tests were succesfull.
>   */
>  bool test();
>
>  /**
>   * Protected method for stress test cases for TestTool.
>   * @return True if tests were succesfull.
>   */
>  bool stressTest();

How is stressTest supposed to work? You *really* need to put more 
thought into writing the comments. Documenting classes isn't easy, 
but it's important especially for a distributed development team.

>/**
>  * Testable is abstract base class providing testing frame for classes.
>  */
>class Testable : public Loggable {
>  friend Tester;

Is the friend declaration needed?

>  /**
>   * Public method for application interal operation tests for startup.
>   * @return True if check was succesfull.
>   */
>  virtual bool systemCheck() = 0;

If this is intended to be called only from within derived classes why 
is it here? If not why isn't it being called?

>  /**
>   * Protected member holding the composte Tester instance.
>   */
>  Tester m_tester;

This isn't initialized and I don't know why anyone would want it.

>/**
>  * TestTool is the static main class of Util test framework.
>  */
>class TestTool : public Loggable {
>  public:
>  /**
>   * Public method for adding tester to system.
>   * @return True if tester name was unique.
>   */
>   static bool add(Tester *tester);

Who deletes the tester? Why isn't this class a singleton?

>  private:
>  /**
>   * Private member map holding tester instances.
>   */
>   static map<string,Tester *> m_testers;

Why bother with both Tester and Testable? Can't the map hold pointers 
to Testable's?

>   Tester::Tester(const string &name,Testable *testable,Logger *logger) :
>Loggable(logger) {
>     assert(testable);
>     m_testable=testable;
>     m_name=name;
>     TestTool::add(this);
>   }

I'm not keen on the idea of introducing a circular dependency between 
Tester and TestTool nor do I think it's a good idea for lower level 
classes (Tester) to know about higher level classes (TestTool).

>   bool  Tester::test() {
>     log(m_name,"test()","Running generic test cases.");
>     bool success=m_testable->test();
>     if(success)
>       log(m_name,"test()","Generic tests passed.");
>     else
>       log(m_name,"test()","Generic tests failed.");
>     return success;
>   }

I don't know about this. I'd use a convention where tests printed a 
message as they run (eg "Starting AIEngine test"). This way you know 
that the tests are actually running which is a nice confidence 
builder. The above code seems like it'll generate a lot of useless 
spam.

>   bool  Tester::stressTest() {
>     /* Add memory consumption check here. */

I don't know if you guys have settled on a token for this sort of 
thing, but you really need something searchable for stuff like this 
(TODO, $$$, whatever).

>   // Logger.clone() is needed so that Loggable instance has own
>instance.
>   TestTool::TestTool(Logger *logger) : Loggable(logger->clone()) {
>   }

Cloning is an awkward and inefficient way to handle this. Using a 
reference counted smart pointer is probably a better way to go.

>   bool TestTool::testAll() {
>     map<string,Tester *>::iterator i;
>     map<string,Tester *>::iterator end=m_testers.end();
>
>     bool success=true;
>     log("TestTool","testAll()","Running generic test cases.");
>     for(i=m_testers.begin();i!=end;i++) {
>       if(!((*i).second->test())) success=false;
>     }

It's much more efficient to use ++i instead of i++ with the more 
complex STL containers (like map). This avoids the creation of a 
temporary which can make a big difference in simple loops.

>     if(success)
>       log("TestTool","testAll()","Generic tests passed.");
>     else
>       log("TestTool","testAll()","Generic tests failed.");
>     return success;
>   }

Why do the log messages say "Generic"? It might be a good idea to 
write out the number of tests, especially for the failures.

>   bool TestTool::add(Tester *tester) {
>     assert(tester);
>     if(m_testers.count(tester->name()))
>       return false;

I find it hard to believe that anyone will check the result of 
calling this method. Better to tighten up the interface and make 
adding a test with a duplicate name a precondition violation, catch 
it with assert, and return void.

>   void TestTool::remove(Tester *tester) {
>     assert(tester);
>     if(m_testers.count(tester->name()))
>       m_testers.erase(m_testers.find(tester->name()));

This too, should be a precondition violation. You also don't need the 
call to find(): "m_testers.erase(tester->name())" will work. For that 
matter, you probably don't need a map at all, especially for a 
server. std::set<Tester*> would probably be better.

   -- Jesse


_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

At 7:25 PM +0100 7/3/01, James Turner wrote:
>On 03 Jul 2001 11:08:31 -0700, Jesse Jones wrote:
>
>>  >   class Logger {
>>  >   public:
>>
>>  >     /**
>>  >      * Dispatches debug messages.
>>  >      * @param m The message string.
>>  >      */
>>
>>  This is a style issue but I really hate comments that convey no
>>  information (eg labeling a dtor or a default ctor) or can be avoided
>>  by tweaking the code (using "mesg" instead of "m").
>
>Actually this is for the benefit of Doxygen :-)

Sure, but there's no reason to document self explanatory members or 
arguments...

>  >  From your other post you see this as working as follows:
>>
>>  class AIEngine : public Loggable {
>>  public:
>>      AIEngine(Logger* log) : Loggable(logger) {}
>>
>>     void think() {this->log("AIEngine", "AIEngine:: think", "Thinking
>>  deep thoughts"); /* do stuff */}
>>  };
>>
>>  Wouldn't it be better to ditch Loggable, the questionable public
>>  inheritance, and rely on composition? Something like this:
>>
>>  class AIEngine  {
>>  public:
>>      ~AIEngine() {delete mLog;}
>>
>>      AIEngine(Logger* takeLog) : mLog (takeLog) {}
>>
>>     void think () {mLog->log("AIEngine : Thinking deep thoughts"); /*
>>  do stuff */}
>>
>>  private:
>>      Logger* mLog;
>>  };
>>
>>  >   class Loggable {
>
>I consider both of these design annoying;

Me too, but the composition approach is at least a bit better than 
the original scheme.

>my requirements are:
>     - logging be globall available; i.e static methods or functions;

Yep

>     the virtualisation should be hidden.

The only virtualness I see needed is to support different sinks which 
most people shouldn't have to worry about.

>     - printf style args

This is pretty easy to do, but it's not type safe and awkward with 
custom types. An iostream approach would seem better, but when I 
tried it I got tremendous bloat in every file that used logging 
(100-200k IIRC). What I do now is rely on overloaded functions:

TRACE(const std::string& mesg);

In debug this calls a singleton logger object to write to zero or 
more sinks (in release it does nothing). Then there are overloads for 
different number of arguments:

template <typename A1>
inline void TRACE(const A1& a1)	   {TRACE(ToStr(a1));}

template <typename A1, typename A2>
inline void TRACE(const A1& a1, const A2& a2)   {TRACE(ToStr(a1) + ToStr(a2));}

There are ToStr functions defined for all the primitive types and you 
can define custom ones for your own types. So you can do things like:

TRACE("bbox = ", mBBox, "\t loc = ", "mLoc);

>So I would like the publci API (obviosuly with a virtual class inside
>to do the clever ness) to look like this:
>
>namespace Log {
>
>Log(const string component, const char *format, ...);
>
>Debug( ... );
>
>};

The other big missing piece is some sort of filtering. In apps with a 
UI it's awfully nice to add logging for all sorts of different things 
and enable just the ones you want (ideally via a string). Server 
software is a different kettle of fish, but I think the same ideas 
apply.

   -- Jesse
_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

On 03 Jul 2001 12:05:02 -0700, Jesse Jones wrote:

> 
> >     - printf style args
> 
> This is pretty easy to do, but it's not type safe and awkward with 
> custom types. An iostream approach would seem better, but when I 
> tried it I got tremendous bloat in every file that used logging 
> (100-200k IIRC). What I do now is rely on overloaded functions:
> 
> TRACE(const std::string& mesg);
> 
> In debug this calls a singleton logger object to write to zero or 
> more sinks (in release it does nothing). Then there are overloads for 
> different number of arguments:
> 
> template <typename A1>
> inline void TRACE(const A1& a1)          {TRACE(ToStr(a1));}
> 
> template <typename A1, typename A2>
> inline void TRACE(const A1& a1, const A2& a2)   {TRACE(ToStr(a1) + ToStr(a2));}
> 
> There are ToStr functions defined for all the primitive types and you 
> can define custom ones for your own types. So you can do things like:
> 
> TRACE("bbox = ", mBBox, "\t loc = ", "mLoc);

Hmm. There are only a few types where this makes sense, but I agree it's
probably worth it. Care to submit a header?


> 
> The other big missing piece is some sort of filtering. In apps with a 
> UI it's awfully nice to add logging for all sorts of different things 
> and enable just the ones you want (ideally via a string). Server 
> software is a different kettle of fish, but I think the same ideas 
> apply.

I don't care about filtering, but of course many people will. So I would
like to be able to set a default, and generally have a simple strategy.

How about a category string, i.e "network" or "db", with the option to
supply multiple values if people feel the need ?

One final thing I usually do is call the logging functions via a macro
which automatically includes __LINE__ and __FILE__; this makes debugging
much less painful if you have similar messages.

I also do this for exceptions, where again it's super-mehga handy. What
annoys me is there aren't equivalent macros for __METHOD__ and
__CLASS__, which would just be too wonderful.

James

_______________________________________________
General mailing list
General@...
https://mail.worldforge.org/lists/listinfo/general

> I'm not sure I like the design of these classes. I think of logging
> as a system service global to the app. But these classes make logging
> an object level policy which means you can't do things like changing
> the log destinations without touching a gazillion places in the code.
>

The idea is exactly that you can change log destinations easily by
specializing logger. For example galileo has own version of Logger
which will directs log calls to Galileos system.

I have not done this logging system before and I don't know any
good pattern for it. This the only easy way I could figure for util
classes to be able to use Galileo for example without creating
dependency and I recall that you suggested something like this
for thread class when we were reviewing it.


> I've never written any server side software, but I imagine logging
> would be pretty important and I don't think the stuff I've seen so
> far is adequate.
>

The actual logging stuff is in Galileo. That what is in util is just a away

util logging stuff can be directed to actual logging system without
creating dependency.

>
> >   /**
> >    * Logger is simple log message dispatcher.
> >    * It can be specialized for different log systems.
> >    */
>
> Better to say something like "Logger is the class that does the
> writing to a particular destination. The default writes to
> std::cout". Dispatcher has the connotation of selecting between
> various destinations.
>

Ok maybe forwarded would be better. English is not my native so
you have to for give me some errors.

>
> >     /**
> >      * Clones Logger instance. Needed to enable virtual cloning.
> >      * @return The new Logger instance.
> >      */
> >     virtual Logger* clone() {
> >       Logger *logger=new Logger();
> >       *logger=*this;
> >       return logger;
> >     }
>
> Clone methods should always be const. It's not safe to rely on the
> assignment operator here (if the object is a subclass and they forget
> to overrride clone() the object will be sliced).
>

Could you give me example how this should be done. This was again
the only way I could figure it could be done.


>
> >     /**
> >      * Dispatches debug messages.
> >      * @param m The message string.
> >      */
>
> This is a style issue but I really hate comments that convey no
> information (eg labeling a dtor or a default ctor) or can be avoided
> by tweaking the code (using "mesg" instead of "m").
>

I am very sorry.


>
> >     virtual void debug(const string &m) {
> >       cout << m << '\n';
> >     }
>
> It's bad style to inline virtual methods. It's even worse to inline
> iostream calls (this will inject massive amounts of object code into
> every file that uses Logger which the linker will eventually dead
> strip). I'm not sure it's wise to add a new line here: it makes it
> awkward for users who want to write lines out piecemeal.
>

Again I am very sorry. I will correct this immediately.


>
> >     /**
> >      * Dispatches log messages.
> >      * @param m The message string.
> >      */
> >     virtual void log(const string &m) {
> >       cout << m << '\n';
> >     }
>
> Do we really need both debug() and log() methods? Wouldn't it be
> better to wrap the debug logging with #if DEBUG?
>

This is to enable sending messages of different levels... very usefull
for galileo.. maybe there could be even more or then add level
atribute to the method but I thought creating separate method
calls with intuitive names are better.


> >   /**
> >    * Loggable is base class for objects using Util logging framework.
> >    * Each Loggable have reference to Logger instance which does log
> >    * dispatching.
> >    */
>
> If you're going to do this you should treat Loggable as a mixin (and
> there should be some agreement on naming and appropriate semantics).
>

This is mostly aimed for util classes. We don't have time to discuss weeks
over each class name.

>
>  From your other post you see this as working as follows:
>
> class AIEngine : public Loggable {
> public:
>     AIEngine(Logger* log) : Loggable(logger) {}
>
>    void think() {this->log("AIEngine", "AIEngine:: think", "Thinking
> deep thoughts"); /* do stuff */}
> };
>
> Wouldn't it be better to ditch Loggable, the questionable public
> inheritance, and rely on composition? Something like this:
>

This is done to make usage of this logging system as easy as possible.
Ie minimal approach.

> Base classes should pretty much always disable the copy ctor and the
> assignment operator.
>

I have never done this. I guess I should learn it after you have complained

about it ten times. Still I don't see so much use of it.


>
> >     /**
> >      * Dispatches debug messages.
> >      * @param name The name of the Loggable instance.
> >      * @param method The method were log message were invoked.
> >      * @param m The message string.
> >      */
> >     virtual void debug(const string &name, const string &method,const
> >string &message); // Used for sending debug messages.
>
> It's not at all clear from the comments what these are for.
> Filtering? Annotating the message? Something else? From the code it
> seems to be annotating.
>
> It might be worthwhile putting a flush() method in here. It's
> important to regularly flush things like file sinks so that you don't
> lose information in the event of a crash, but doing it on every line
> might have performance implications.
>

mm.. sounds like an idea.. gotta talk with Lee about it.

It would be encouraging if you would give also some positive remarks on a
code review. If there is any =T.

rgs,
Morgy

_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

> I consider both of these design annoying; my requirements are:
>     - logging be globall available; i.e static methods or functions;
>     the virtualisation should be hidden.
>
>     - printf style args
>
> So I would like the publci API (obviosuly with a virtual class inside
> to do the clever ness) to look like this:
>
> namespace Log {
>
> Log(const string component, const char *format, ...);
>
> Debug( ... );
>
> };
>
>

As I replied to Jesses post already this is logging framework is only for
util
library so that the logging can be either forwarded to console or for
example galileo. I don't think this can be done well enough with any static
approach. For example in stage log messages have also code which tells
which system send to log message. Now when for example thread is used
in certain sub system the specialized logger of that thread knows the code
of the system and puts it in when it sends the message to galileo.

I cannot do this printf hacking. Someone else needs to write it if we want
such
chocolate=).

_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

> The other big missing piece is some sort of filtering. In apps with a
> UI it's awfully nice to add logging for all sorts of different things
> and enable just the ones you want (ideally via a string). Server
> software is a different kettle of fish, but I think the same ideas
> apply.
>

Ok I propably screwed this thing by calling it logging framework.
Stupid me. It is just log forwaring system to forward the logs to the
actual logging system.

_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

Hello people,

this weeks review will handle a couple of experimental frameworks in the
Util directory which will become Util library at some point. The
experimental frameworks for review are logging and testing frameworks. I
have included the source files + doxygen generated help files as
attachment. I will also give source files as independent replies to this
message so it is possible to comment them in separate threads.

Maybe it would be good idea to require doxygen generated documents for
all reviews from this on. Opinions?

rgs,
Tommi Laukkanen
(Morgenes)

> How are these tests invoked? A separate test app? When the server
> starts up? Have you looked at any of the prior art on unit test
> frameworks?

Look into sys/util for example program.

I have not looked to any unit testing stuff. I just wrote something which
enables to run tests on systems. I don't have like weeks to explore
this stuff. If someone has the time he is most wellcome to do it and
write something more like state of art.

> This isn't initialized and I don't know why anyone would want it.

It is initialized. Composite members are initialized automaticly AFAIK.

>
> Why bother with both Tester and Testable? Can't the map hold pointers
> to Testable's?
>

The actual reason to put the interface to separate class was that when
stressTest() is called from the interface it calls virtual testable->stressTest()
so that the right child classes method is called instead that of Testable which =
0.

Maybe I could have done this also so that I put the interface in Testable but
don't call from interface:
stressTest();
but
this->stressTest();
Does this invoke the child classes method and not the = 0 of Testable?
I hope you can follow what I mean. Anyway this->stressTest() did not
occur to me back when I was doing this so making separate Tester class
was what I figured would work. Anyway this will be changed if this->
works..

>
> >   Tester::Tester(const string &name,Testable *testable,Logger *logger) :
> >Loggable(logger) {
> >     assert(testable);
> >     m_testable=testable;
> >     m_name=name;
> >     TestTool::add(this);
> >   }
>
> I'm not keen on the idea of introducing a circular dependency between
> Tester and TestTool nor do I think it's a good idea for lower level
> classes (Tester) to know about higher level classes (TestTool).
>

This is done so Testers can register themselves to TestTool. I think
you are too afraid of circular depencies. Sometimes they are usefull
and AFAIK it is ok to use them inside package.

> I don't know about this. I'd use a convention where tests printed a
> message as they run (eg "Starting AIEngine test"). This way you know
> that the tests are actually running which is a nice confidence
> builder. The above code seems like it'll generate a lot of useless
> spam.
>

Agreed. When the system is working these can be stripped out.

>
> >   bool  Tester::stressTest() {
> >     /* Add memory consumption check here. */
>

This is private note. There is common convention for public notes..

> >   // Logger.clone() is needed so that Loggable instance has own
> >instance.
> >   TestTool::TestTool(Logger *logger) : Loggable(logger->clone()) {
> >   }
>
> Cloning is an awkward and inefficient way to handle this. Using a
> reference counted smart pointer is probably a better way to go.
>

I don't like making a nuclear missile to kill an insect.

> It's much more efficient to use ++i instead of i++ with the more
> complex STL containers (like map). This avoids the creation of a

Noted!

> temporary which can make a big difference in simple loops.
>
> >     if(success)
> >       log("TestTool","testAll()","Generic tests passed.");
> >     else
> >       log("TestTool","testAll()","Generic tests failed.");
> >     return success;
> >   }
>
> Why do the log messages say "Generic"? It might be a good idea to
> write out the number of tests, especially for the failures.
>

The implementation in Testable inherited class will do this.

>
> >   bool TestTool::add(Tester *tester) {
> >     assert(tester);
> >     if(m_testers.count(tester->name()))
> >       return false;
>
> I find it hard to believe that anyone will check the result of
> calling this method. Better to tighten up the interface and make
> adding a test with a duplicate name a precondition violation, catch
> it with assert, and return void.
>

Yeah that sounds better.

>
> >   void TestTool::remove(Tester *tester) {
> >     assert(tester);
> >     if(m_testers.count(tester->name()))
> >       m_testers.erase(m_testers.find(tester->name()));
>
> This too, should be a precondition violation. You also don't need the
> call to find(): "m_testers.erase(tester->name())" will work. For that
> matter, you probably don't need a map at all, especially for a
> server. std::set<Tester*> would probably be better.
>

Right.

rgs,
Morgenes

_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

At 9:40 AM -0700 7/4/01, Tommi Laukkanen wrote:
>  > I'm not sure I like the design of these classes. I think of logging
>>  as a system service global to the app. But these classes make logging
>>  an object level policy which means you can't do things like changing
>>  the log destinations without touching a gazillion places in the code.
>>
>
>The idea is exactly that you can change log destinations easily by
>specializing logger. For example galileo has own version of Logger
>which will directs log calls to Galileos system.

The problem is that the loggers are attached to individual objects 
which makes it a lot more difficult to change the destination because 
there are so many places in the code to change.

>  > >     /**
>>  >      * Clones Logger instance. Needed to enable virtual cloning.
>>  >      * @return The new Logger instance.
>>  >      */
>>  >     virtual Logger* clone() {
>>  >       Logger *logger=new Logger();
>>  >       *logger=*this;
>>  >       return logger;
>>  >     }
>>
>>  Clone methods should always be const. It's not safe to rely on the
>>  assignment operator here (if the object is a subclass and they forget
>>  to overrride clone() the object will be sliced).
>>
>
>Could you give me example how this should be done. This was again
>the only way I could figure it could be done.

Just keep it the way it is and assign the members manually.

>  > >     /**
>  > >      * Dispatches log messages.
>>  >      * @param m The message string.
>>  >      */
>>  >     virtual void log(const string &m) {
>>  >       cout << m << '\n';
>>  >     }
>>
>>  Do we really need both debug() and log() methods? Wouldn't it be
>>  better to wrap the debug logging with #if DEBUG?
>>
>
>This is to enable sending messages of different levels... very usefull
>for galileo.. maybe there could be even more or then add level
>atribute to the method but I thought creating separate method
>calls with intuitive names are better.

Different levels, or debug and non-debug messages?

>  > >   /**
>>  >    * Loggable is base class for objects using Util logging framework.
>>  >    * Each Loggable have reference to Logger instance which does log
>>  >    * dispatching.
>>  >    */
>>
>>  If you're going to do this you should treat Loggable as a mixin (and
>>  there should be some agreement on naming and appropriate semantics).
>>
>
>This is mostly aimed for util classes. We don't have time to discuss weeks
>over each class name.

It's not a naming issue. It's a class design issue. What if you want 
to add logging to a class that already inherits from something? Then 
you have to use multiple inheritance which opens up a nasty can of 
worms. Mixins give you a way to avoid the worst of the problems.

>  >  From your other post you see this as working as follows:
>>
>>  class AIEngine : public Loggable {
>>  public:
>>      AIEngine(Logger* log) : Loggable(logger) {}
>>
>>     void think() {this->log("AIEngine", "AIEngine:: think", "Thinking
>>  deep thoughts"); /* do stuff */}
>>  };
>>
>>  Wouldn't it be better to ditch Loggable, the questionable public
>>  inheritance, and rely on composition? Something like this:
>>
>
>This is done to make usage of this logging system as easy as possible.
>Ie minimal approach.

Minimal is good, but isn't my idea even simpler?

>  > Base classes should pretty much always disable the copy ctor and the
>>  assignment operator.
>>
>
>I have never done this. I guess I should learn it after you have complained
>
>about it ten times. Still I don't see so much use of it.

The problem is that it's impossible to write a copy ctor and an 
assignment operator that will always work with objects in an 
inheritance hierarchy. And the compiler generated versions work even 
less often. Since it's very easy to accidentally try to copy an 
object it's a Good Thing if you can arrange to get a compile time 
error if you try this. You do this by making the copy ctor and the 
assignment operator private (so you'll get a compiler error) and 
undefined (so you'll get a linker error if you somehow manage to call 
them).

   -- Jesse
_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

>I cannot do this printf hacking. Someone else needs to write it if we want
>such
>chocolate=).

You do something like this:

void DEBUGSTR(const char* format, ...)
{
    char mesg[2048];

    va_list args;
    va_start(args, format);
    vsprintf(mesg, format, args);
    va_end(args);

    BreakStrToDebugger(mesg);
}

   -- Jesse
_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

On 04 Jul 2001 01:38:52 -0700, Jesse Jones wrote:

> You do something like this:
> 
> void DEBUGSTR(const char* format, ...)
> {
>     char mesg[2048];
> 
>     va_list args;
>     va_start(args, format);
>     vsprintf(mesg, format, args);
>     va_end(args);
> 
>     BreakStrToDebugger(mesg);
> }
>

Your Mac heritage is shinning through ... alas; to the best of my
knowledge, Unix has no equivalent to Debug() and DebugStr().
The best they can do is assert(), which is non-restartable. Sigh.

It's amazing, over the past 3 years I've done Unix and Win32, what's I'd
have given to have functions that did this.

But the point about vargs is fine, although I suspect your template
version with ToString() calls is more viable. (So we can do something
intelligent for Atlas objects and Vectors).

James.

_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

At 10:06 AM -0700 7/4/01, Tommi Laukkanen wrote:
>  > How are these tests invoked? A separate test app? When the server
>>  starts up? Have you looked at any of the prior art on unit test
>>  frameworks?
>
>Look into sys/util for example program.

Hmm, it looks like separate apps. I've always been uncomfortable with 
this. It's hard to ensure that the tests are run regularly and they 
run in a different environment so you can never be sure if things 
like optimizer settings are going to screw you over.

>I have not looked to any unit testing stuff. I just wrote something which
>enables to run tests on systems. I don't have like weeks to explore
>this stuff. If someone has the time he is most wellcome to do it and
>write something more like state of art.

This is important stuff and well worth spending a bit of time looking 
at what others have done. I can give you some links if you like.

>  > This isn't initialized and I don't know why anyone would want it.
>
>It is initialized. Composite members are initialized automaticly AFAIK.

Member data will be constructed with the default ctor, but the 
primitive types don't have a default ctor so they're left 
uninitialized. Why is it there at all?

>  >
>>  Why bother with both Tester and Testable? Can't the map hold pointers
>>  to Testable's?
>>
>
>The actual reason to put the interface to separate class was that when
>stressTest() is called from the interface it calls virtual 
>testable->stressTest()
>so that the right child classes method is called instead that of 
>Testable which =
>0.

But AFAICT Tester doesn't do *anything*. All it does is delegate all 
the work to Testable. Why not get rid of it?

>Maybe I could have done this also so that I put the interface in Testable but
>don't call from interface:
>stressTest();
>but
>this->stressTest();
>Does this invoke the child classes method and not the = 0 of Testable?

There is absolutely no difference between the two. The only 
difference is that using "this->" makes it clear that you're calling 
a member function. That's why I use it (although most people don't).

>I hope you can follow what I mean. Anyway this->stressTest() did not
>occur to me back when I was doing this so making separate Tester class
>was what I figured would work. Anyway this will be changed if this->
>works..

And what was stressTest() for?

>  > >   Tester::Tester(const string &name,Testable *testable,Logger *logger) :
>>  >Loggable(logger) {
>>  >     assert(testable);
>>  >     m_testable=testable;
>>  >     m_name=name;
>  > >     TestTool::add(this);
>>  >   }
>>
>>  I'm not keen on the idea of introducing a circular dependency between
>>  Tester and TestTool nor do I think it's a good idea for lower level
>>  classes (Tester) to know about higher level classes (TestTool).
>>
>
>This is done so Testers can register themselves to TestTool. I think
>you are too afraid of circular depencies. Sometimes they are usefull
>and AFAIK it is ok to use them inside package.

Maybe you're right (and in fact this is what I do in my own unit test 
code). It looks bogus to have  TestTool have a mix of static and 
non-static methods though. If you're going to do this sort of thing 
you should do it right and use a singleton.

>  > >   bool  Tester::stressTest() {
>>  >     /* Add memory consumption check here. */
>>
>
>This is private note. There is common convention for public notes..

You mean a private note to yourself? Are you sure that you're going 
to clean all of the private notes up? If not I'd get in the habit of 
using the special token...

>  > >   // Logger.clone() is needed so that Loggable instance has own
>>  >instance.
>>  >   TestTool::TestTool(Logger *logger) : Loggable(logger->clone()) {
>>  >   }
>>
>>  Cloning is an awkward and inefficient way to handle this. Using a
>  > reference counted smart pointer is probably a better way to go.
>>
>
>I don't like making a nuclear missile to kill an insect.

Actually, in thinking about this, I wasn't quite right. What you 
would do is make Logger ref counted. This is slightly more 
complicated than using a reference counted smart pointer class (which 
is dirt simple). Is cloning even going to work? What if the logger is 
writing to a file? Won't there be problems writing to the same file?

>  > >     if(success)
>>  >       log("TestTool","testAll()","Generic tests passed.");
>>  >     else
>>  >       log("TestTool","testAll()","Generic tests failed.");
>>  >     return success;
>>  >   }
>>
>>  Why do the log messages say "Generic"? It might be a good idea to
>>  write out the number of tests, especially for the failures.
>>
>
>The implementation in Testable inherited class will do this.

But this is in TestTool, the code in Testable won't affect this...

   -- Jesse
_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

> At 10:06 AM -0700 7/4/01, Tommi Laukkanen wrote:
> >  > How are these tests invoked? A separate test app? When the server
> >>  starts up? Have you looked at any of the prior art on unit test
> >>  frameworks?
> >
> >Look into sys/util for example program.
>
> Hmm, it looks like separate apps. I've always been uncomfortable with
> this. It's hard to ensure that the tests are run regularly and they
> run in a different environment so you can never be sure if things
> like optimizer settings are going to screw you over.
>

I really don't follow you here. I made a example application but you can
also integrate the stuff to stage or run only systemCheck() stuff from stage.


>
> >I have not looked to any unit testing stuff. I just wrote something which
> >enables to run tests on systems. I don't have like weeks to explore
> >this stuff. If someone has the time he is most wellcome to do it and
> >write something more like state of art.
>
> This is important stuff and well worth spending a bit of time looking
> at what others have done. I can give you some links if you like.
>

Paste away I look at them if I got time.

>
> >  > This isn't initialized and I don't know why anyone would want it.
> >
> >It is initialized. Composite members are initialized automaticly AFAIK.
>
> Member data will be constructed with the default ctor, but the
> primitive types don't have a default ctor so they're left
> uninitialized. Why is it there at all?
>

Tester m_tester;  This is not primitive so it will get initialized.
It is there because testers are composite parts of Testables. In other
words one testable always has one Tester as composite.

> But AFAICT Tester doesn't do *anything*. All it does is delegate all
> the work to Testable. Why not get rid of it?
>

I see that you don't follow what I was saying.

>
> >Maybe I could have done this also so that I put the interface in Testable but
> >don't call from interface:
> >stressTest();
> >but
> >this->stressTest();
> >Does this invoke the child classes method and not the = 0 of Testable?
>
> There is absolutely no difference between the two. The only
> difference is that using "this->" makes it clear that you're calling
> a member function. That's why I use it (although most people don't).
>

Again seems that you did not get my point.

>
> >I hope you can follow what I mean. Anyway this->stressTest() did not
> >occur to me back when I was doing this so making separate Tester class
> >was what I figured would work. Anyway this will be changed if this->
> >works..
>
> And what was stressTest() for?
>

test() runs generic test cases where as stressTest runs time/memory consumption
and memory leakage tests etc. tester->stressTest() calls stressTest() of linked
Testable class which has specialized the stressTest() method to execute wanted
stress test cases.


>
> >  > >   Tester::Tester(const string &name,Testable *testable,Logger *logger) :
> >>  >Loggable(logger) {
> >>  >     assert(testable);
> >>  >     m_testable=testable;
> >>  >     m_name=name;
> >  > >     TestTool::add(this);
> >>  >   }
> >>
> >>  I'm not keen on the idea of introducing a circular dependency between
> >>  Tester and TestTool nor do I think it's a good idea for lower level
> >>  classes (Tester) to know about higher level classes (TestTool).
> >>
> >
> >This is done so Testers can register themselves to TestTool. I think
> >you are too afraid of circular depencies. Sometimes they are usefull
> >and AFAIK it is ok to use them inside package.
>
> Maybe you're right (and in fact this is what I do in my own unit test
> code). It looks bogus to have  TestTool have a mix of static and
> non-static methods though. If you're going to do this sort of thing
> you should do it right and use a singleton.
>

Bogus or not it works fine. I figure I could make it singleton as well.
Anyway you are running only version of TestTool per app.


>
> >  > >   bool  Tester::stressTest() {
> >>  >     /* Add memory consumption check here. */
> >>
> >
> >This is private note. There is common convention for public notes..
>
> You mean a private note to yourself? Are you sure that you're going
> to clean all of the private notes up? If not I'd get in the habit of
> using the special token...
>

Yeah thanks mom.

>
> >  > >   // Logger.clone() is needed so that Loggable instance has own
> >>  >instance.
> >>  >   TestTool::TestTool(Logger *logger) : Loggable(logger->clone()) {
> >>  >   }
> >>
> >>  Cloning is an awkward and inefficient way to handle this. Using a
> >  > reference counted smart pointer is probably a better way to go.
> >>
> >
> >I don't like making a nuclear missile to kill an insect.
>
> Actually, in thinking about this, I wasn't quite right. What you
> would do is make Logger ref counted. This is slightly more
> complicated than using a reference counted smart pointer class (which
> is dirt simple). Is cloning even going to work? What if the logger is
> writing to a file? Won't there be problems writing to the same file?
>

If you had read my earlier message you would have noticed that logger
forwards the log messages to the real messaging system like Galileo.
The reason to create this logging system was to be able to forward the
log messages generated in the util library to the apps logging system
without making dependency to any specific logging system. If someone
has better idea please reply it.

>
> >  > >     if(success)
> >>  >       log("TestTool","testAll()","Generic tests passed.");
> >>  >     else
> >>  >       log("TestTool","testAll()","Generic tests failed.");
> >>  >     return success;
> >>  >   }
> >>
> >>  Why do the log messages say "Generic"? It might be a good idea to
> >>  write out the number of tests, especially for the failures.
> >>
> >
> >The implementation in Testable inherited class will do this.
>
> But this is in TestTool, the code in Testable won't affect this...
>

Again we have some kind of communication problem here. Do you mean
that the number of registered Testers should be given? TestTool does
not know how many generic test cases some certain Testable will do. It
depends on the implementation in the Testable inherited class.

rgs,
Morgenes

_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

At 2:24 PM -0700 7/4/01, Tommi Laukkanen wrote:
>  > At 10:06 AM -0700 7/4/01, Tommi Laukkanen wrote:
>>  >  > How are these tests invoked? A separate test app? When the server
>>  >>  starts up? Have you looked at any of the prior art on unit test
>>  >>  frameworks?
>>  >
>>  >Look into sys/util for example program.
>>
>>  Hmm, it looks like separate apps. I've always been uncomfortable with
>>  this. It's hard to ensure that the tests are run regularly and they
>>  run in a different environment so you can never be sure if things
>>  like optimizer settings are going to screw you over.
>>
>
>I really don't follow you here. I made a example application but you can
>also integrate the stuff to stage or run only systemCheck() stuff from stage.

All I'm saying is that I think it would be best if Stage ran these 
tests as it started up.

>  > >I have not looked to any unit testing stuff. I just wrote something which
>>  >enables to run tests on systems. I don't have like weeks to explore
>>  >this stuff. If someone has the time he is most wellcome to do it and
>>  >write something more like state of art.
>>
>>  This is important stuff and well worth spending a bit of time looking
>>  at what others have done. I can give you some links if you like.
>>
>
>Paste away I look at them if I got time.

Here are some pointers to unit test code:

1) Here's the code I use: 
<ftp://ftp.halcyon.com/pub/users/jesjones/Unit Tests.zip>. It's nice 
and simple, but it's geared towards a UI app where you can select 
which test(s) you want to run from a menu so it probably does a bit 
more than you want.

2) Boost doesn't yet have a unit test framework, but it does have a 
test framework for stand alone applications. Boost is one of the very 
best C++ libraries out there so it may be worth checking out. See 
<http://www.boost.org/libs/test/doc/execution_tools.htm>.

3) The Extreme Programming community is into unit testing in a big 
way. They have a unit test framework for C++ called CppUnit. You can 
find it at <http://www.xprogramming.com/software.htm> (click on the 
"C++ Ver 1.5" link). I think it's a little heavy weight, but it's 
certainly worth checking out.

There's a lot of logging code out there and I've read quite a few 
articles, but I haven't seen much that's very good. You can find the 
code I use at <ftp://ftp.halcyon.com/pub/users/jesjones/Logging.zip>. 
XDebug.h has the main interface (and some more stuff you may not care 
about):

TRACE(...) writes its arguments to zero or more sinks using ToStr() 
overloads to convert the arguments to strings.

TRACEFLOW(const char* category, ...) is like TRACE() except that it 
supports filtering: the arguments are only written if the category is 
enabled.

XTrace.h has the singleton object that bottle-necks the writes. It 
keeps track of which categories are enabled and maintains the list of 
trace sinks.

XTraceSink.h has the abstract base class for the various sinks. 
XTraceSinks.h has file and console like sinks.

>  > >  > This isn't initialized and I don't know why anyone would want it.
>>  >
>>  >It is initialized. Composite members are initialized automaticly AFAIK.
>>
>>  Member data will be constructed with the default ctor, but the
>>  primitive types don't have a default ctor so they're left
>>  uninitialized. Why is it there at all?
>>
>
>Tester m_tester;  This is not primitive so it will get initialized.
>It is there because testers are composite parts of Testables. In other
>words one testable always has one Tester as composite.

By god you're right. I thought it was a pointer to the containing 
object which makes some sort of sense. But I don't follow this at 
all. What we have is:

TestTool which maintains a list of Tester objects.

Tester which has a pointer to a Testable object. Tester doesn't seem 
to do anything useful: it delegates all the important stuff to 
Testable.

Testable which is an abstract base class which users specialize to do 
their tests.

So, why does Testable contain a Tester *object*? I can't see how it 
makes any sense for a Testable to be associated with two distinct 
Tester objects...

>  > But AFAICT Tester doesn't do *anything*. All it does is delegate all
>>  the work to Testable. Why not get rid of it?
>>
>
>I see that you don't follow what I was saying.

I have trouble following your prose sometimes, but the code seems 
clear enough. Do you disagree that Tester can be deleted? Or do you 
think that's a bad idea?

>  > >Maybe I could have done this also so that I put the interface in 
>Testable but
>>  >don't call from interface:
>>  >stressTest();
>>  >but
>  > >this->stressTest();
>>  >Does this invoke the child classes method and not the = 0 of Testable?
>>
>>  There is absolutely no difference between the two. The only
>>  difference is that using "this->" makes it clear that you're calling
>>  a member function. That's why I use it (although most people don't).
>>
>
>Again seems that you did not get my point.

this->stressTest() works just like stressTest() so it will use the 
vtable to find the function to call. As long as you're not inside a 
ctor or dtor you'll be OK calling a pure virtual method.

>  >
>>  >I hope you can follow what I mean. Anyway this->stressTest() did not
>>  >occur to me back when I was doing this so making separate Tester class
>>  >was what I figured would work. Anyway this will be changed if this->
>>  >works..
>>
>>  And what was stressTest() for?
>>
>
>test() runs generic test cases where as stressTest runs time/memory 
>consumption
>and memory leakage tests etc. tester->stressTest() calls 
>stressTest() of linked
>Testable class which has specialized the stressTest() method to execute wanted
>stress test cases.

Why is this in the base class? Won't stress testing be meaningless 
for most unit tests? And for the few that may benefit from stress 
tests can't the derived class handle it?

>  > Maybe you're right (and in fact this is what I do in my own unit test
>>  code). It looks bogus to have  TestTool have a mix of static and
>>  non-static methods though. If you're going to do this sort of thing
>>  you should do it right and use a singleton.
>>
>
>Bogus or not it works fine. I figure I could make it singleton as well.
>Anyway you are running only version of TestTool per app.

But TestTool is essentially a singleton right now: you can make 
multiple copies of TestTool but they all have the same state so it 
doesn't matter which you deal with. Given this you can make the code 
clearer and simpler by using the appropriate design pattern.

>  > >  > >   // Logger.clone() is needed so that Loggable instance has own
>>  >>  >instance.
>>  >>  >   TestTool::TestTool(Logger *logger) : Loggable(logger->clone()) {
>>  >>  >   }
>>  >>
>>  >>  Cloning is an awkward and inefficient way to handle this. Using a
>>  >  > reference counted smart pointer is probably a better way to go.
>>  >>
>>  >
>>  >I don't like making a nuclear missile to kill an insect.
>>
>>  Actually, in thinking about this, I wasn't quite right. What you
>  > would do is make Logger ref counted. This is slightly more
>>  complicated than using a reference counted smart pointer class (which
>>  is dirt simple). Is cloning even going to work? What if the logger is
>>  writing to a file? Won't there be problems writing to the same file?
>>
>
>If you had read my earlier message you would have noticed that logger
>forwards the log messages to the real messaging system like Galileo.
>The reason to create this logging system was to be able to forward the
>log messages generated in the util library to the apps logging system
>without making dependency to any specific logging system.

I don't think this is germane to the point I was raising: 1) you have 
no control over what a Logger subclass may want to do. You can 
require them to support clone() but this seems like a needless 
burden. 2) The real point is that you don't want to clone loggers at 
all. You want to share one logger among multiple objects. The easiest 
way to do this is to ref count them.

>If someone
>has better idea please reply it.

I don't think you should worry about dependencies on Galileo. Logging 
is one of those low level services that many many modules will wind 
up depending on.

>  > >  > >     if(success)
>>  >>  >       log("TestTool","testAll()","Generic tests passed.");
>  > >>  >     else
>>  >>  >       log("TestTool","testAll()","Generic tests failed.");
>>  >>  >     return success;
>>  >>  >   }
>>  >>
>>  >>  Why do the log messages say "Generic"? It might be a good idea to
>>  >>  write out the number of tests, especially for the failures.
>>  >>
>>  >
>>  >The implementation in Testable inherited class will do this.
>>
>>  But this is in TestTool, the code in Testable won't affect this...
>>
>
>Again we have some kind of communication problem here. Do you mean
>that the number of registered Testers should be given? TestTool does
>not know how many generic test cases some certain Testable will do. It
>depends on the implementation in the Testable inherited class.

Lets say ten Tester objects were processed and two had errors. 
Presumably the bad ones will assert or log the problems. After they 
all run TestTool will print "Generic tests failed". Why does it say 
"Generic"? We didn't run a single generic test. In fact "generic 
test" is a complete oxymoron.

Wouldn't it be better if TestTool printed something like this: "10 
unit tests were run, 2 failed"?

   -- Jesse
_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

At 9:55 AM +0100 7/4/01, James Turner wrote:
>On 04 Jul 2001 01:38:52 -0700, Jesse Jones wrote:
>
>>  You do something like this:
>>
>>  void DEBUGSTR(const char* format, ...)
>>  {
>>      char mesg[2048];
>>
>>      va_list args;
>>      va_start(args, format);
>>      vsprintf(mesg, format, args);
>>      va_end(args);
>>
>>      BreakStrToDebugger(mesg);
>>  }
>>
>
>Your Mac heritage is shinning through ... alas; to the best of my
>knowledge, Unix has no equivalent to Debug() and DebugStr().
>The best they can do is assert(), which is non-restartable. Sigh.
>
>It's amazing, over the past 3 years I've done Unix and Win32, what's I'd
>have given to have functions that did this.

On Windows you can do this:

define some inline assembly to break into the debugger:

#if MSVC
     #define MyDebugBreak() _asm {int 3}
#else
     #define MyDebugBreak() asm {int 3}
#endif

then do something like the following:

OutputDebugString(mesg);
MyDebugBreak();

This will crash if you're not running with the debugger, but you can 
check for that (although it's icky to do on Win95). I can give you 
some code if you want.

   -- Jesse
_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

Jesse Jones wrote:

> I'm not sure I like the design of these classes. I think of logging
> as a system service global to the app. But these classes make logging
> an object level policy which means you can't do things like changing
> the log destinations without touching a gazillion places in the code.

I'm not sure how well it applies, but Xerox PARC's Aspect-Oriented
Programming paradigm might make logging easier to deal with.  It's
definitely worth a look, and may suggest some better approaches to logging.

  http://www.parc.xerox.com/csl/projects/aop/

John

--
John R. Sheets, Consultant
jsheets@...
McCaa, Webster & Associates, Inc.

Writing GNOME Applications
http://www.aw.com/cseng/titles/0-201-65791-0/



_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server

Jesse Jones wrote:

> >   bool  Tester::stressTest() {
> >     /* Add memory consumption check here. */
>
> I don't know if you guys have settled on a token for this sort of
> thing, but you really need something searchable for stuff like this
> (TODO, $$$, whatever).

I'd strongly recommend "FIXME" for incomplete or missing core features, and "TODO"
for optional extra features.  That's basically what I've been using in COAL/Anvil
(to varying degrees of consistency, errhum).

John

--
John R. Sheets, Consultant
jsheets@...
McCaa, Webster & Associates, Inc.

Writing GNOME Applications
http://www.aw.com/cseng/titles/0-201-65791-0/




_______________________________________________
Server mailing list
Server@...
http://mail.worldforge.org/lists/listinfo/server