commenbts

13 files changed, 77 insertions, 52 deletions

diff --git a/cpp/src/IcePack/ActivatorI.cpp b/cpp/src/IcePack/ActivatorI.cpp
index dd11db49854..f8e7a7f66b2 100644
--- a/cpp/src/IcePack/ActivatorI.cpp
+++ b/cpp/src/IcePack/ActivatorI.cpp
@@ -158,6 +158,11 @@ IcePack::ActivatorI::activate(const ServerPrx& server, const ::Ice::Current&)
 	// Ice logger could be changed to flush the stream and not use
 	// automatic flushing.
 	//
+
+// TODO: ML: Can you be more specific? Sending messages through a pipe
+// is one of the most basic unix mechanisms. I don't see how this
+// could not work.
+
 // 	if(fds[1] != STDERR_FILENO)
 // 	{
 // 	    if(dup2(fds[1], STDERR_FILENO) != STDERR_FILENO)
diff --git a/cpp/src/IcePack/ActivatorI.h b/cpp/src/IcePack/ActivatorI.h
index bc5064d02c2..ad95a039afc 100644
--- a/cpp/src/IcePack/ActivatorI.h
+++ b/cpp/src/IcePack/ActivatorI.h
@@ -15,6 +15,8 @@
 #include <IcePack/Activator.h>

 #include <IcePack/ServerManagerF.h>

 

+// TODO: ML: This file has DOS line endings. Change to Unix line endings.

+

 namespace IcePack

 {

 

diff --git a/cpp/src/IcePack/ComponentDeployer.cpp b/cpp/src/IcePack/ComponentDeployer.cpp
index 36e9699a839..d5973d57fde 100644
--- a/cpp/src/IcePack/ComponentDeployer.cpp
+++ b/cpp/src/IcePack/ComponentDeployer.cpp
@@ -69,7 +69,7 @@ public:
 	if(mkdir(_name.c_str(), 0755) != 0)
 	{
 	    DeploymentException ex;
-	    ex.reason = "Couldn't create directory " + _name + ": " + strerror(getSystemErrno());
+	    ex.reason = "couldn't create directory " + _name + ": " + strerror(getSystemErrno());
 	    throw ex;
 	}
     }
@@ -178,7 +178,7 @@ public:
 	if(!configfile)
 	{
 	    DeploymentException ex;
-	    ex.reason = "Couldn't create configuration file: " + _file;
+	    ex.reason = "couldn't create configuration file: " + _file;
 	    throw ex;
 	}
 	
@@ -228,7 +228,7 @@ public:
 	catch(const Ice::LocalException& lex)
 	{
 	    ostringstream os;
-	    os << "Couldn't contact the yellow service: " << lex << endl;
+	    os << "couldn't contact Yellow: " << lex << endl;
 
 	    OfferDeploymentException ex;
 	    ex.reason = os.str();
@@ -383,7 +383,7 @@ IcePack::ComponentDeployHandler::getAttributeValue(const AttributeList& attrs, c
     
     if(value == 0)
     {
-	throw DeploySAXParseException("Missing attribute '" + name + "'", _locator);
+	throw DeploySAXParseException("missing attribute '" + name + "'", _locator);
     }
 
     return _deployer.substitute(toString(value));
@@ -518,7 +518,10 @@ IcePack::ComponentDeployer::parse(const string& xmlFile, ComponentDeployHandler&
 	delete parser;
 
 	ostringstream os;
-	os << xmlFile << ": UnknownException while parsing file.";
+	// TODO: ML: UnknownException? This it not UnknownException,
+	// that's an unknown exception. UnknownException is an Ice
+	// exception.
+	os << xmlFile << ": UnknownException while parsing file";
 
 	ParserDeploymentException ex;
 	ex.component = _variables["name"];
@@ -533,11 +536,12 @@ IcePack::ComponentDeployer::parse(const string& xmlFile, ComponentDeployHandler&
     {
 	ParserDeploymentException ex;
 	ex.component = _variables["name"];
-	ex.reason = xmlFile + ": Parser returned non null error count";
+	ex.reason = xmlFile + ": parse error";
 	throw ex;
     }    
 }
 
+// TODO: ML: Add space, loose last const.
 void
 IcePack::ComponentDeployer::setDocumentLocator(const Locator*const locator)
 {
@@ -648,7 +652,7 @@ IcePack::ComponentDeployer::substitute(const string& v) const
 	
 	if(end == string::npos)
 	{
-	    throw DeploySAXParseException("Malformed variable name in the '" + value + "' value", _locator);
+	    throw DeploySAXParseException("malformed variable name in the '" + value + "' value", _locator);
 	}
 
 	
@@ -656,7 +660,7 @@ IcePack::ComponentDeployer::substitute(const string& v) const
 	map<string, string>::const_iterator p = _variables.find(name);
 	if(p == _variables.end())
 	{
-	    throw DeploySAXParseException("Unknown variable name in the '" + value + "' value", _locator);
+	    throw DeploySAXParseException("unknown variable name in the '" + value + "' value", _locator);
 	}
 
 	value.replace(beg, end - beg + 1, p->second);
diff --git a/cpp/src/IcePack/ComponentDeployer.h b/cpp/src/IcePack/ComponentDeployer.h
index c9e6ad07b16..1e3b1f06eaa 100644
--- a/cpp/src/IcePack/ComponentDeployer.h
+++ b/cpp/src/IcePack/ComponentDeployer.h
@@ -31,6 +31,9 @@ public:
     virtual void undeploy() = 0;
 };
 
+// TODO: ML: Nonportable. The incRef/decRef declarations must be done
+// before the typedef below. Why not just use IceUtil::Handle in this
+// case?
 typedef ::IceInternal::Handle< ::IcePack::Task> TaskPtr;
 
 void incRef(::IcePack::Task*);
@@ -42,6 +45,9 @@ class DeploySAXParseException : public SAXParseException
 {
 public:
 
+    // TODO: ML: Space is missing: "const Locator* const
+    // locator". Also, why not just const Locator*? What's the point
+    // of making the pointer constant as well?
     DeploySAXParseException(const std::string&, const Locator*const locator);
 
 };
@@ -83,10 +89,16 @@ public:
 
     ComponentDeployHandler(ComponentDeployer&);
 
+    // TODO: ML: Incorrect spacing, should be "const XMLCh*
+    // const". Also, loose the last const -- there is no point in
+    // forcing the pointer itself to be const. (Here and everywhere
+    // else.)
     virtual void characters(const XMLCh *const, const unsigned int);
+    // TODO: ML: AttributeList&.
     virtual void startElement(const XMLCh *const, AttributeList &); 
     virtual void endElement(const XMLCh *const);
 
+    // TODO: ML: No reason to make inline, see style guide.
     virtual void ignorableWhitespace(const XMLCh *const, const unsigned int) { }
     virtual void processingInstruction(const XMLCh *const, const XMLCh *const) { }
     virtual void resetDocument() { }
diff --git a/cpp/src/IcePack/LocatorI.h b/cpp/src/IcePack/LocatorI.h
index 5825c2374b9..206c5344bfb 100644
--- a/cpp/src/IcePack/LocatorI.h
+++ b/cpp/src/IcePack/LocatorI.h
@@ -20,7 +20,7 @@ namespace IcePack
 
 class LocatorI : public ::Ice::Locator
 {
-public:
+public: // TODO: ML: Add empty line.
     LocatorI(const AdapterManagerPrx&, const ::Ice::LocatorRegistryPrx&);
 
     virtual ::Ice::ObjectPrx findAdapterByName(const std::string&, const ::Ice::Current&);
diff --git a/cpp/src/IcePack/LocatorRegistryI.h b/cpp/src/IcePack/LocatorRegistryI.h
index fa035860de5..bf53fb5d0ef 100644
--- a/cpp/src/IcePack/LocatorRegistryI.h
+++ b/cpp/src/IcePack/LocatorRegistryI.h
@@ -21,7 +21,7 @@ namespace IcePack
 
 class LocatorRegistryI : public Ice::LocatorRegistry
 {
-public:
+public: // TODO: ML: Add empty line.
     LocatorRegistryI(const AdapterManagerPrx&);
     
     virtual void addAdapter(const ::std::string&, const ::Ice::ObjectPrx&, const ::Ice::Current&);
diff --git a/cpp/src/IcePack/Parser.cpp b/cpp/src/IcePack/Parser.cpp
index 245bb940d0c..c709b0abaaa 100644
--- a/cpp/src/IcePack/Parser.cpp
+++ b/cpp/src/IcePack/Parser.cpp
@@ -41,20 +41,19 @@ void
 IcePack::Parser::usage()
 {
     cout <<
-        "help                        Print this message.\n"
-        "exit, quit                  Exit this program.\n"
-        "server add NAME PATH LIBRARY_PATH DESCRIPTOR\n"
-        "                            Add server NAME with PATH.\n"
-	"server describe NAME        Get server NAME description.\n"
-	"server state NAME           Get server NAME state.\n"
-	"server start NAME           Starts server NAME.\n"
-        "server remove NAME          Remove server NAME.\n"
-        "server list                 List all server names.\n"
-	"adapter add NAME ENDPOINTS  Add adapter NAME with ENDPOINTS.\n"
-        "adapter list                List all adapter names.\n"
-        "adapter remove NAME         Remove adapter NAME.\n"
-	"adapter endpoints NAME      Get adapter NAME endpoints.\n"
-        "shutdown                    Shut the IcePack server down.\n";
+        "help                                Print this message.\n"
+        "exit, quit                          Exit this program.\n"
+        "server add NAME PATH LIBPATH DESC   Add server NAME with PATH.\n" // TODO: ML: What about LIBPATH, DESC?
+	"server describe NAME                Get server NAME description.\n"
+	"server state NAME                   Get server NAME state.\n"
+	"server start NAME                   Starts server NAME.\n"
+        "server remove NAME                  Remove server NAME.\n"
+        "server list                         List all server names.\n"
+	"adapter add NAME ENDPOINTS          Add adapter NAME with ENDPOINTS.\n"
+        "adapter list                        List all adapter names.\n"
+        "adapter remove NAME                 Remove adapter NAME.\n"
+	"adapter endpoints NAME              Get adapter NAME endpoints.\n"
+        "shutdown                            Shut the IcePack server down.\n";
 }
 
 void
@@ -63,7 +62,7 @@ IcePack::Parser::addServer(const list<string>& args, const std::list<std::string
 {
     if(args.size() != 4)
     {
-	error("`server add' requires four arguments (type `help' for more info)");
+	error("`server add' requires four arguments\n(`help' for more info)");
 	return;
     }
 
@@ -103,7 +102,7 @@ IcePack::Parser::startServer(const list<string>& args)
 {
     if(args.size() != 1)
     {
-	error("`server start' requires exactly one argument (type `help' for more info)");
+	error("`server start' requires exactly one argument\n(`help' for more info)");
 	return;
     }
 
@@ -111,6 +110,9 @@ IcePack::Parser::startServer(const list<string>& args)
     {
 	if(!_admin->startServer(args.front()))
 	{
+	    // TODO: ML: That's not in our style guide, but don't
+	    // capitalize error messages, i.e., use "the server
+	    // didn't...". (Here and in other places.)
 	    error("The server didn't start successfully");
 	}
     }
@@ -127,7 +129,7 @@ IcePack::Parser::describeServer(const list<string>& args)
 {
     if(args.size() != 1)
     {
-	error("`server describe' requires exactly one argument (type `help' for more info)");
+	error("`server describe' requires exactly one argument\n(`help' for more info)");
 	return;
     }
 
@@ -160,7 +162,7 @@ IcePack::Parser::stateServer(const list<string>& args)
 {
     if(args.size() != 1)
     {
-	error("`server state' requires exactly one argument (type `help' for more info)");
+	error("`server state' requires exactly one argument\n(`help' for more info)");
 	return;
     }
 
@@ -170,20 +172,21 @@ IcePack::Parser::stateServer(const list<string>& args)
 
 	switch(state)
 	{
+	    // TODO: ML: Indentation wrong, { } missing.
 	case Inactive:
-	    cout << "Inactive" << endl;
+	    cout << "inactive" << endl;
 	    break;
 	case Activating:
-	    cout << "Activating" << endl;
+	    cout << "activating" << endl;
 	    break;
 	case Active:
-	    cout << "Active" << endl;
+	    cout << "active" << endl;
 	    break;
 	case Deactivating:
-	    cout << "Deactivating" << endl;
+	    cout << "deactivating" << endl;
 	    break;
 	case Destroyed:
-	    cout << "Destroyed" << endl;
+	    cout << "destroyed" << endl;
 	    break;
 	default:
 	    assert(false);
@@ -202,7 +205,7 @@ IcePack::Parser::removeServer(const list<string>& args)
 {
     if(args.size() != 1)
     {
-	error("`server remove' requires exactly one argument (type `help' for more info)");
+	error("`server remove' requires exactly one argument\n(`help' for more info)");
 	return;
     }
 
@@ -239,7 +242,7 @@ IcePack::Parser::addAdapter(const list<string>& args)
 {
     if(args.size() < 2)
     {
-	error("`adapter add' requires at least two arguments (type `help' for more info)");
+	error("`adapter add' requires at least two arguments\n(`help' for more info)");
 	return;
     }
 
@@ -265,7 +268,7 @@ IcePack::Parser::endpointsAdapter(const list<string>& args)
 {
     if(args.size() != 1)
     {
-	error("`adapter endpoints' requires exactly one argument (type `help' for more info)");
+	error("`adapter endpoints' requires exactly one argument\n(`help' for more info)");
 	return;
     }
 
@@ -287,7 +290,7 @@ IcePack::Parser::removeAdapter(const list<string>& args)
 {
     if(args.size() != 1)
     {
-	error("`adapter remove' requires exactly one argument (type `help' for more info)");
+	error("`adapter remove' requires exactly one argument\n(`help' for more info)");
 	return;
     }
 
diff --git a/cpp/src/IcePack/Server.cpp b/cpp/src/IcePack/Server.cpp
index 16502266c9c..fefa7aada7f 100644
--- a/cpp/src/IcePack/Server.cpp
+++ b/cpp/src/IcePack/Server.cpp
@@ -124,7 +124,7 @@ main(int argc, char* argv[])
     }
     else
     {
-	cerr << argv[0] << ": IcePack.Data doesn't contain a valid directory path." << endl;
+	cerr << argv[0] << ": `IcePack.Data' doesn't contain a valid directory path." << endl;
 	return EXIT_FAILURE;
     }
 
diff --git a/cpp/src/IcePack/ServerDeployer.cpp b/cpp/src/IcePack/ServerDeployer.cpp
index 608d39c697d..de78b8726af 100644
--- a/cpp/src/IcePack/ServerDeployer.cpp
+++ b/cpp/src/IcePack/ServerDeployer.cpp
@@ -44,13 +44,13 @@ public:
 	{
 	    AdapterDeploymentException ex;
 	    ex.adapter = _desc.name;
-	    ex.reason = "Adapter already exist";
+	    ex.reason = "adapter already exist";
 	    throw ex;
 	}
 	catch(const Ice::LocalException& lex)
 	{
 	    ostringstream os;
-	    os << "Couldn't contact the adpater manager: " << lex << endl;
+	    os << "couldn't contact the adpater manager: " << lex << endl;
 
 	    AdapterDeploymentException ex;
 	    ex.adapter = _desc.name;
@@ -277,7 +277,7 @@ IcePack::ServerDeployer::setClassName(const string& name)
 
     if(name.empty())
     {
-	throw DeploySAXParseException("Empty classname element value", _locator);
+	throw DeploySAXParseException("empty classname element value", _locator);
     }
 
     _className = name;
@@ -288,7 +288,7 @@ IcePack::ServerDeployer::setWorkingDirectory(const string& pwd)
 {
     if(pwd.empty())
     {
-	throw DeploySAXParseException("Empty working directory", _locator);
+	throw DeploySAXParseException("no working directory", _locator);
     }
 
     _description.pwd = pwd;
@@ -304,7 +304,7 @@ IcePack::ServerDeployer::addAdapter(const string& name, const string& endpoints)
 
     if(name.empty())
     {
-	throw DeploySAXParseException("Empty adapter name", _locator);
+	throw DeploySAXParseException("no adapter name", _locator);
     }
 
     AdapterDescription desc;
@@ -327,16 +327,17 @@ IcePack::ServerDeployer::addService(const string& name, const string& descriptor
 {
     if(_kind != ServerKindCppIceBox && _kind !=  ServerKindJavaIceBox)
     {
-	throw DeploySAXParseException("Service are only allows in IceBox servers", _locator);
+	// TODO: ML: Grammar. Should this be "allowed"?
+	throw DeploySAXParseException("services are only allows in IceBox servers", _locator);
     }
 
     if(name.empty())
     {
-	throw DeploySAXParseException("Name attribute value is empty", _locator);
+	throw DeploySAXParseException("name attribute value is empty", _locator);
     }
     if(descriptor.empty())
     {
-	throw DeploySAXParseException("Descriptor attribute value is empty", _locator);
+	throw DeploySAXParseException("descriptor attribute value is empty", _locator);
     }
 
     //
diff --git a/cpp/src/IcePack/ServerManagerI.cpp b/cpp/src/IcePack/ServerManagerI.cpp
index 890d73c582b..6dd9a240582 100644
--- a/cpp/src/IcePack/ServerManagerI.cpp
+++ b/cpp/src/IcePack/ServerManagerI.cpp
@@ -58,9 +58,6 @@ public:
     {
     }
 
-    //
-    // Operations from ObjectFactory
-    //
     virtual Ice::ObjectPtr 
     create(const std::string& type)
     {
@@ -106,7 +103,7 @@ IcePack::ServerI::start(const Current&)
     while(true)
     {
 	IceUtil::Monitor< IceUtil::Mutex>::Lock sync(*this);
-	if(!_activator)
+	if(!_activator) // TODO: ML: { } missing, here and in several places below.
 	    return false;
 
 	switch(_state)
diff --git a/cpp/src/IcePack/ServerManagerI.h b/cpp/src/IcePack/ServerManagerI.h
index 9ee4105c56d..e4ca7816e09 100644
--- a/cpp/src/IcePack/ServerManagerI.h
+++ b/cpp/src/IcePack/ServerManagerI.h
@@ -23,7 +23,7 @@ namespace IcePack
 
 class ServerI : public Server, public ::IceUtil::Monitor< ::IceUtil::Mutex>
 {
-public:
+public: // TODO: ML: Empty line missing.
     ServerI(const ::Ice::ObjectAdapterPtr&, const ActivatorPrx&);
     virtual ~ServerI();
     
diff --git a/cpp/src/IcePack/ServiceDeployer.cpp b/cpp/src/IcePack/ServiceDeployer.cpp
index 817799f6b1d..6c39de8f07d 100644
--- a/cpp/src/IcePack/ServiceDeployer.cpp
+++ b/cpp/src/IcePack/ServiceDeployer.cpp
@@ -131,7 +131,7 @@ IcePack::ServiceDeployer::setDBEnv(const string& dir)
 {
     if(_kind != ServiceKindFreeze)
     {
-	throw DeploySAXParseException("Database environment is only allowed for Freeze services", _locator);
+	throw DeploySAXParseException("database environment is only allowed for Freeze services", _locator);
     }
 
     string path;
diff --git a/cpp/src/IcePack/ServiceDeployer.h b/cpp/src/IcePack/ServiceDeployer.h
index 6895d868585..e72af463eb7 100644
--- a/cpp/src/IcePack/ServiceDeployer.h
+++ b/cpp/src/IcePack/ServiceDeployer.h
@@ -13,6 +13,7 @@
 
 #include <IceUtil/Shared.h>
 #include <IcePack/ComponentDeployer.h>
+// Remove duplicate empty line.
 
 
 namespace IcePack