comments

9 files changed, 68 insertions, 44 deletions

diff --git a/cpp/demo/Ice/session/Client.cpp b/cpp/demo/Ice/session/Client.cpp
index 64022d4fbd9..84bc1d15e97 100755
--- a/cpp/demo/Ice/session/Client.cpp
+++ b/cpp/demo/Ice/session/Client.cpp
@@ -9,7 +9,7 @@
 
 #include <Ice/Ice.h>
 #include <IceUtil/Thread.h>
-#include <Session.h>

+#include <Session.h>
 
 using namespace std;
 using namespace Demo;
@@ -64,6 +64,7 @@ private:
     const IceUtil::Time _timeout;
     bool _terminated;
 };
+
 typedef IceUtil::Handle<SessionRefreshThread> SessionRefreshThreadPtr;
 
 void
@@ -71,9 +72,9 @@ menu()
 {
     cout <<
 	"usage:\n"
-	"c:     create new hello\n"
-	"0-9:   greet hello object\n"
-	"s:     shutdown server\n"
+	"c:     create a new per-client hello object\n"
+	"0-9:   send a greeting to a hello object\n"
+	"s:     shutdown the server\n"
 	"x:     exit\n"
 	"t:     exit without destroying the session\n"
 	"?:     help\n";
@@ -132,14 +133,14 @@ run(int argc, char* argv[], const Ice::CommunicatorPtr& communicator)
 		}
 		else
 		{
-		    cout << "index is too high. " << hellos.size() << " exist so far. "
-			 << "Use 'c' to create a new hello object." << endl;
+		    cout << "Index is too high. " << hellos.size() << " hello objects exist so far.\n"
+			 << "Use `c' to create a new hello object." << endl;
 		}
 	    }
 	    else if(c == 'c')
 	    {
 		hellos.push_back(session->createHello());
-		cout << "created hello object " << hellos.size()-1 << endl;
+		cout << "Created hello object #" << hellos.size() - 1 << endl;
 	    }
 	    else if(c == 's')
 	    {
@@ -160,7 +161,7 @@ run(int argc, char* argv[], const Ice::CommunicatorPtr& communicator)
 	    }
 	    else
 	    {
-		cout << "unknown command `" << c << "'" << endl;
+		cout << "Unknown command `" << c << "'." << endl;
 		menu();
 	    }
 	}
@@ -181,6 +182,8 @@ run(int argc, char* argv[], const Ice::CommunicatorPtr& communicator)
     return EXIT_SUCCESS;
 }
 
+// XXX Rewrite to use Application. See ../callback for an example.
+
 int
 main(int argc, char* argv[])
 {
diff --git a/cpp/demo/Ice/session/ReapThread.cpp b/cpp/demo/Ice/session/ReapThread.cpp
index 1cd8bae6f8b..9380aee84fa 100755
--- a/cpp/demo/Ice/session/ReapThread.cpp
+++ b/cpp/demo/Ice/session/ReapThread.cpp
@@ -20,6 +20,7 @@ ReapThreadPtr&
 ReapThread::instance()
 {
     IceUtil::StaticMutex::Lock sync(_instanceMutex);
+
     if(!_instance)
     {
 	_instance = new ReapThread;
@@ -36,11 +37,16 @@ void
 ReapThread::run()
 {
     Lock sync(*this);
+
     while(!_terminated)
     {
 	timedWait(_timeout);
+
 	if(!_terminated)
 	{
+	    // XXX Session destruction may take time in a real-world
+	    // example. Therefore now should be computed in the loop
+	    // for each iteration.
 	    IceUtil::Time now = IceUtil::Time::now();
 	    map<SessionPrx, SessionIPtr>::iterator p = _sessions.begin();
 	    while(p != _sessions.end())
@@ -52,6 +58,7 @@ ReapThread::run()
 			cout << "The session #" << Ice::identityToString(p->first->ice_getIdentity())
 			     << " has timed out." << endl;
 			p->first->destroy();
+			// XXX This can be simplified to _sessions.erase(p++);
 			map<SessionPrx, SessionIPtr>::iterator tmp = p;
 			++p;
 			_sessions.erase(tmp);
@@ -63,6 +70,7 @@ ReapThread::run()
     	    	}
 		catch(const Ice::ObjectNotExistException&)
 		{
+		    // XXX This can be simplified to _sessions.erase(p++);
 		    map<SessionPrx, SessionIPtr>::iterator tmp = p;
 		    ++p;
 		    _sessions.erase(tmp);
@@ -76,12 +84,11 @@ void
 ReapThread::terminate()
 {
     Lock sync(*this);
+
     _terminated = true;
     notify();
 
-    for(map<SessionPrx, SessionIPtr>::const_iterator p = _sessions.begin();
-	p != _sessions.end();
-	++p)
+    for(map<SessionPrx, SessionIPtr>::const_iterator p = _sessions.begin(); p != _sessions.end(); ++p)
     {
 	try
 	{
@@ -89,9 +96,10 @@ ReapThread::terminate()
 	}
 	catch(const Ice::Exception&)
 	{
-	    // Ignore the exception
+	    // Ignore.
 	}
     }
+
     _sessions.clear();
 }
 
@@ -99,6 +107,9 @@ void
 ReapThread::add(const SessionPrx& proxy, const SessionIPtr& session)
 {
     Lock sync(*this);
+    // XXX Don't use make_pair, it's not portable. We had to remove it
+    // from most of our code, because the Sun compiler and others had
+    // problems with it.y
     _sessions.insert(make_pair(proxy, session));
 }
 
diff --git a/cpp/demo/Ice/session/ReapThread.h b/cpp/demo/Ice/session/ReapThread.h
index 7667e8b16dd..012dac82f47 100755
--- a/cpp/demo/Ice/session/ReapThread.h
+++ b/cpp/demo/Ice/session/ReapThread.h
@@ -22,27 +22,13 @@ public:
 
     static ReapThreadPtr& instance();
 
-    virtual ~ReapThread();
+    virtual ~ReapThread(); // XXX Destructor does nothing, get rid of it.
 
     virtual void run();
 
-    // XXX Rename to destroy().
-    //
-    // I named it terminate because destroy() methods in Java result
-    // in a deprecation warning. If you want to use destroy() in Java
-    // then I would have to use a runnable which means the demo isn't
-    // the same code. Given the difference is a method name, I didn't
-    // think it was worth the cost of different code.
-    //
     void terminate();
 
-    // XXX: The alternative here is to make timestamp() a slice
-    // method. However, this means that we're adding methods to the
-    // slice interface which is only required for the reaping thread,
-    // and it obviously couldn't return an IceUtil::Time but instead
-    // some other representation...
-    //
-    void add(const ::Demo::SessionPrx&, const SessionIPtr&);
+    void add(const Demo::SessionPrx&, const SessionIPtr&);
 
 private:
 
@@ -50,7 +36,10 @@ private:
 
     const IceUtil::Time _timeout;
     bool _terminated;
-    std::map< ::Demo::SessionPrx, SessionIPtr> _sessions;
+    // XXX Why is this a map and not simply a list of
+    // pair<Demo::SessionPrx, SessionIPtr> (or a list of structs with
+    // these elements)? The sorting of a map is needed nowhere.
+    std::map< Demo::SessionPrx, SessionIPtr> _sessions;
 
     static ReapThreadPtr _instance;
     static IceUtil::StaticMutex _instanceMutex;
diff --git a/cpp/demo/Ice/session/Server.cpp b/cpp/demo/Ice/session/Server.cpp
index 3c5da0ef43a..6199c3d5025 100644
--- a/cpp/demo/Ice/session/Server.cpp
+++ b/cpp/demo/Ice/session/Server.cpp
@@ -7,7 +7,7 @@
 //
 // **********************************************************************
 
-#include <Ice/Ice.h>
+#include <Ice/Ice.h> // XXX Get rid of this, see other comments.
 #include <SessionFactoryI.h>
 #include <ReapThread.h>
 
@@ -18,10 +18,12 @@ int
 run(int argc, char* argv[], const Ice::CommunicatorPtr& communicator)
 {
     Ice::ObjectAdapterPtr adapter = communicator->createObjectAdapter("SessionFactory");
+
     ReapThreadPtr reaper = ReapThread::instance();
+    reaper->start();
+
     adapter->add(new SessionFactoryI, Ice::stringToIdentity("SessionFactory"));
     adapter->activate();
-    reaper->start();
     communicator->waitForShutdown();
 
     reaper->terminate();
diff --git a/cpp/demo/Ice/session/Session.ice b/cpp/demo/Ice/session/Session.ice
index 564960b9a23..3b887b64f26 100644
--- a/cpp/demo/Ice/session/Session.ice
+++ b/cpp/demo/Ice/session/Session.ice
@@ -15,15 +15,21 @@ module Demo
 
 interface Hello
 {
-    // Send a greeting.
     nonmutating void sayHello();
 };
 
 //
 // The session object. This is used to create per-session objects on
-// behalf of the client. If it is not refreshed on a periodic basis it
+// behalf of the client. If it is not refreshed on a periodic basis, it
 // will be automatically reclaimed by the session factory.
-///
+//
+// XXX Comment is wrong. First, sessions are not reclaimed - they are
+// destroyed. Second, the factory doesn't do this. Third, it is
+// irrelevant for the interface description to explain what
+// programming language construct exactly destroys expired
+// sessions. So this should simply read: "If it is not refreshed on a
+// periodic basis, it will be automatically destroyed."
+//
 interface Session
 {
     //
@@ -34,18 +40,22 @@ interface Session
 
     //
     // Refresh a session. If a session is not refreshed on a regular
-    // basis by the client it will be automatically destroyed.
+    // basis by the client, it will be automatically destroyed.
     //
     idempotent void refresh();
 
     //
-    // Destroy the session.
+    // Destroy the session explicitly.
     //
     void destroy();
 };
 
 interface SessionFactory
 {
+    // XXX I think you should pass a user name parameter in here. The
+    // client could then first ask "What is your name"? Then the
+    // server could print messages like "Hello object #XXX says `Hello
+    // Foo!'"
     Session* create();
 
     idempotent void shutdown();
diff --git a/cpp/demo/Ice/session/SessionFactoryI.cpp b/cpp/demo/Ice/session/SessionFactoryI.cpp
index 66d27f083e8..03fa652e862 100755
--- a/cpp/demo/Ice/session/SessionFactoryI.cpp
+++ b/cpp/demo/Ice/session/SessionFactoryI.cpp
@@ -7,7 +7,7 @@
 //
 // **********************************************************************
 
-#include <Ice/Ice.h>
+#include <Ice/Ice.h> // XXX Get rid of this, SessionFactoryI.cpp must include this.
 #include <SessionFactoryI.h>
 #include <ReapThread.h>
 
@@ -25,7 +25,7 @@ SessionFactoryI::~SessionFactoryI()
 SessionPrx
 SessionFactoryI::create(const Ice::Current& c)
 {
-    Lock sync(*this);
+    Lock sync(*this); // XXX What is the mutex lock needed for?
 
     SessionIPtr session = new SessionI;
     SessionPrx proxy = SessionPrx::uncheckedCast(c.adapter->addWithUUID(session));
@@ -34,7 +34,7 @@ SessionFactoryI::create(const Ice::Current& c)
 }
 
 void
-SessionFactoryI::shutdown(const ::Ice::Current& c)
+SessionFactoryI::shutdown(const Ice::Current& c)
 {
     cout << "Shutting down..." << endl;
     c.adapter->getCommunicator()->shutdown();
diff --git a/cpp/demo/Ice/session/SessionFactoryI.h b/cpp/demo/Ice/session/SessionFactoryI.h
index 6987d7f1d22..c46c11c1b2d 100755
--- a/cpp/demo/Ice/session/SessionFactoryI.h
+++ b/cpp/demo/Ice/session/SessionFactoryI.h
@@ -10,16 +10,18 @@
 #ifndef SESSION_FACTORY_I_H
 #define SESSION_FACTORY_I_H
 
+// XXX Missing #includes, see comment in SessionI.h.
 #include <Session.h>
 
-class SessionFactoryI : public Demo::SessionFactory, public IceUtil::Mutex
+class SessionFactoryI : public Demo::SessionFactory, public IceUtil::Mutex // XXX What is the mutex needed for?
 {
 public:
 
+    // XXX Constructor and destructor do nothing, get rid of them.
     SessionFactoryI();
     virtual ~SessionFactoryI();
 
-    virtual Demo::SessionPrx create(const ::Ice::Current&);
+    virtual Demo::SessionPrx create(const Ice::Current&);
     virtual void shutdown(const Ice::Current&);
 };
 
diff --git a/cpp/demo/Ice/session/SessionI.cpp b/cpp/demo/Ice/session/SessionI.cpp
index 4cf07aa8fa3..803c5530036 100755
--- a/cpp/demo/Ice/session/SessionI.cpp
+++ b/cpp/demo/Ice/session/SessionI.cpp
@@ -7,8 +7,8 @@
 //
 // **********************************************************************
 
-#include <Ice/Ice.h>
-#include <SessionFactoryI.h>
+#include <Ice/Ice.h> // XXX Remove, see comment in SessionFactoryI.cpp
+#include <SessionFactoryI.h> // XXX Remove, not needed anywhere.
 #include <SessionI.h>
 
 using namespace std;
@@ -76,6 +76,7 @@ SessionI::destroy(const Ice::Current& c)
 
     _destroy = true;
 
+    // XXX Why "#"? Use "The session with the identity `...' is now destroyed."
     cout << "The session #" << Ice::identityToString(c.id) << " is now destroyed." << endl;
     try
     {
diff --git a/cpp/demo/Ice/session/SessionI.h b/cpp/demo/Ice/session/SessionI.h
index 7988e46078d..30f5b923832 100755
--- a/cpp/demo/Ice/session/SessionI.h
+++ b/cpp/demo/Ice/session/SessionI.h
@@ -10,6 +10,11 @@
 #ifndef SESSION_I_H
 #define SESSION_I_H
 
+// XXX Missing #includes. The #includes for the header must be
+// self-contained, i.e. there must be no problem if you just do
+// #include<SessionFactoryI.h> from an empty .cpp file. (I know that
+// some other demos don't follow this rule, but they need to be
+// fixed. We shouldn't propagate such mistakes into new demos.)
 #include <Session.h>
 #include <list>
 
@@ -32,7 +37,8 @@ private:
     SessionI();
 
     IceUtil::Time _timestamp; // The last time the session was refreshed.
-    
+
+    // XXX This needs to be a static, otherwise hello objects from different client have the same ID.
     int _nextId; // The id of the next hello object. This is used for tracing purposes.
     std::list< Demo::HelloPrx> _objs; // List of per-client allocated Hello objects.
     bool _destroy;