rewrite Properties

let's be rid of some egregiously bad C++ code...

src/utils/configuration.cpp

@@ -24,211 +24,109 @@ namespace pvAccess {
 using namespace epics::pvData;
 using namespace std;
 
-Properties::Properties()
-{
-	_fileName = "";
-	_infile.reset(new ifstream());
-	_infile->exceptions (ifstream::failbit | ifstream::badbit );
-	_outfile.reset(new ofstream());
-	_outfile->exceptions (ofstream::failbit | ofstream::badbit );
-}
+Properties::Properties() {}
 
-Properties::Properties(const string &fileName)
-{
-	_fileName = fileName;
-	_infile.reset(new ifstream());
-	_infile->exceptions (ifstream::failbit | ifstream::badbit );
-	_outfile.reset(new ofstream());
-	_outfile->exceptions (ofstream::failbit | ofstream::badbit );
-}
+Properties::Properties(const string &fileName) : _fileName(fileName) {}
 
-Properties::~Properties()
+const std::string &Properties::getProperty(const string &key) const
 {
-}
-
-void Properties::setProperty(const string &key, const string &value)
-{
-	string oldValue;
-	std::map<std::string,std::string>::iterator propertiesIterator = _properties.find(key);
-
-	if(propertiesIterator != _properties.end()) //found in map
-	{
-		_properties.erase(propertiesIterator);
-	}
-	_properties[key] = value;
-}
-
-string Properties::getProperty(const string &key)
-{
-	std::map<std::string,std::string>::iterator propertiesIterator = _properties.find(key);
-	if(propertiesIterator != _properties.end()) //found in map
-	{
-		return string(propertiesIterator->second);
-	}
-	else
-	{
-		string errMsg = "Property not found in the map: " + key;
-		THROW_BASE_EXCEPTION(errMsg.c_str());
+    _properties_t::const_iterator propertiesIterator = _properties.find(key);
+    if(propertiesIterator != _properties.end()) {
+        return propertiesIterator->second;
+    } else {
+        THROW_BASE_EXCEPTION(string("Property not found in the map: ") + key);
 	}
 }
 
-string Properties::getProperty(const string &key, const string &defaultValue)
+const std::string &Properties::getProperty(const string &key, const string &defaultValue) const
 {
-	std::map<std::string,std::string>::iterator propertiesIterator = _properties.find(key);
-	if(propertiesIterator != _properties.end()) //found in map
-	{
-		return string(propertiesIterator->second);
-	}
-
-	return defaultValue;
+    _properties_t::const_iterator propertiesIterator = _properties.find(key);
+    if(propertiesIterator != _properties.end()) {
+        return propertiesIterator->second;
+    } else {
+        return defaultValue;
+    }
 }
 
-bool Properties::hasProperty(const string &key)
+void Properties::Properties::load()
 {
-    return (_properties.find(key) != _properties.end());
-}
-
-void Properties::load()
-{
-	_properties.clear();
-
-#ifdef NO_STREAM_EXCEPTIONS
-	_infile->open(_fileName.c_str(),ifstream::in);
-	if (_infile->fail())
-#else
-	try
-	{
-		_infile->open(_fileName.c_str(),ifstream::in);
-	}
-	catch (ifstream::failure& e)
-#endif
-	{
-		string errMsg = "Error opening file: " + string(_fileName.c_str());
-		THROW_BASE_EXCEPTION(errMsg.c_str());
-	}
-
-	string line;
-	string property;
-	string key;
-#ifndef NO_STREAM_EXCEPTIONS
-	try
-	{
-#endif
-		while(!_infile->eof())
-		{
-			line.erase();
-			std::getline(*_infile,line);
-
-#ifdef NO_STREAM_EXCEPTIONS
-			if (_infile->fail())
-			{
-				_infile->close();
-				if(_infile->eof())
-				{
-					return; //end of file
-				}
-				string errMsg = "Error reading file: " + _fileName;
-				THROW_BASE_EXCEPTION(errMsg.c_str());
-			}
-#endif
-
-			//remove trailing spaces
-			truncate(line);
-
-			//empty line
-			if(line.length() == 0)
-			{
-				continue;
-			}
-			// comment
-			if(line[0] == '#')
-			{
-				continue;
-			}
-
-			//line is in format: propertyName=propertyValue
-			size_t pos = line.find_first_of('=',0);
-			if(pos == string::npos) //bad value (= not found)
-			{
-				string errMsg = "Bad property line found: " + line;
-				THROW_BASE_EXCEPTION(errMsg.c_str());
-			}
-
-			key = line.substr(0,pos);
-			truncate(key);
-			property = line.substr(pos + 1,line.length());
-			truncate(property);
-			_properties[key] = property;
-		}
-#ifndef NO_STREAM_EXCEPTIONS
-	}
-	catch (ifstream::failure& e)
-	{
-		_infile->close();
-		if(_infile->eof())
-		{
-			return; //end of file
-		}
-		string errMsg = "Error reading file: " + _fileName;
-		THROW_BASE_EXCEPTION(errMsg.c_str());
-	}
-#endif
-	_infile->close();
+    load(_fileName);
 }
 
 void Properties::load(const string &fileName)
 {
-	_fileName = fileName;
-	load();
+    ifstream strm(fileName.c_str());
+    load(strm);
 }
 
-void Properties::store()
+namespace {
+string trim(const string& in)
 {
-#ifdef NO_STREAM_EXCEPTIONS
-	_outfile->open(_fileName.c_str(),ifstream::trunc);
-	if (_outfile->fail())
-#else
-	try
-	{
-		_outfile->open(_fileName.c_str(),ifstream::trunc);
-	}
-	catch (ofstream::failure& e)
-#endif
-	{
-		string errMsg = "Error opening file: " + string(_fileName.c_str());
-		THROW_BASE_EXCEPTION(errMsg.c_str());
-	}
-
-
-	for (std::map<std::string,std::string>::iterator propertiesIterator = _properties.begin();
-			propertiesIterator != _properties.end();
-			propertiesIterator++ )
-	{
-#ifndef NO_STREAM_EXCEPTIONS
-		try
-		{
-#endif
-			string line = string(propertiesIterator->first) + string("=") + string(propertiesIterator->second) + string("\n");
-			_outfile->write(line.c_str(),line.length());
-#ifdef NO_STREAM_EXCEPTIONS
-			if(_outfile->fail())
-#else
-		}
-		catch (ofstream::failure& e)
-#endif
-		{
-			_outfile->close();
-			string errMsg = "Error writing to file: " + string(_fileName.c_str());
-			THROW_BASE_EXCEPTION(errMsg.c_str());
-		}
-	}
-	_outfile->close();
+    size_t A = in.find_first_not_of(" \t\r"),
+           B = in.find_last_not_of(" \t\r");
+    if(A==B)
+        return string();
+    else
+        return in.substr(A, B-A+1);
+}
 }
 
-void Properties::store(const string &fileName)
+void Properties::load(std::istream& strm)
 {
-	_fileName = fileName;
-	store();
+    _properties_t newmap;
+
+    std::string line;
+    unsigned lineno = 0;
+    while(getline(strm, line).good()) {
+        lineno++;
+        size_t idx = line.find_first_not_of(" \t\r");
+        if(idx==line.npos || line[idx]=='#')
+            continue;
+
+        idx = line.find_first_of('=');
+        if(idx==line.npos) {
+            ostringstream msg;
+            msg<<"Malformed line "<<lineno<<" expected '='";
+            throw runtime_error(msg.str());
+        }
+
+        string key(trim(line.substr(0, idx))),
+               value(trim(line.substr(idx+1)));
+
+        if(key.empty()) {
+            ostringstream msg;
+            msg<<"Malformed line "<<lineno<<" expected name before '='";
+            throw runtime_error(msg.str());
+        }
+
+        newmap[key] = value;
+    }
+    if(strm.bad()) {
+        ostringstream msg;
+        msg<<"Malformed line "<<lineno<<" I/O error";
+        throw runtime_error(msg.str());
+    }
+    _properties.swap(newmap);
+}
+
+void Properties::store() const
+{
+    store(_fileName);
+}
+
+void Properties::store(const std::string& fname) const
+{
+    ofstream strm(fname.c_str());
+    store(strm);
+}
+
+void Properties::store(std::ostream& strm) const
+{
+    for(_properties_t::const_iterator it=_properties.begin(), end=_properties.end();
+        it!=end && strm.good(); ++it)
+    {
+        strm << it->first << " = " << it->second << "\n";
+    }
 }
 
 void Properties::list()

src/utils/configuration.h

@@ -41,36 +41,27 @@ class epicsShareClass Properties
 public:
 	Properties();
 	Properties(const std::string &fileName);
-	virtual ~Properties();
 
-	void setProperty(const std::string &key,const std::string &value);
-	std::string getProperty(const std::string &key);
-	std::string getProperty(const std::string &key, const std::string &defaultValue);
-    bool hasProperty(const std::string &key);
+    inline void setProperty(const std::string &key,const std::string &value)
+    { _properties[key] = value; }
+    const std::string& getProperty(const std::string &key) const;
+    const std::string& getProperty(const std::string &key, const std::string &defaultValue) const;
+    inline bool hasProperty(const std::string &key) const
+    { return _properties.find(key) != _properties.end(); }
 
-	void store();
-	void store(const std::string &fileName);
-	void load();
+    void store() const;
+    void store(const std::string &fileName) const;
+    void store(std::ostream& strm) const;
+    void load();
 	void load(const std::string &fileName);
+    void load(std::istream& strm);
 	void list();
 
+    size_t size() const {return _properties.size();}
 private:
-	std::map<std::string,std::string> _properties;
-	std::auto_ptr<std::ifstream> _infile;
-	std::auto_ptr<std::ofstream> _outfile;
-	std::string _fileName;
-
-	inline void	truncate(std::string& str)
-	{
-		while(str.length() != 0 && (str.at(0) == ' ' || str.at(0) == '\t'))
-		{
-			str.erase(0,1);
-		}
-		while(str.length() != 0 && (str.at(str.length()-1) == ' ' || str.at(str.length()-1) == '\t'))
-		{
-			str.erase(str.length()-1,1);
-		}
-	}
+    typedef std::map<std::string,std::string> _properties_t;
+    _properties_t _properties;
+    std::string _fileName;
 };
 
 

testApp/utils/configurationTest.cpp

@@ -14,6 +14,7 @@
 #include <epicsAssert.h>
 #include <epicsExit.h>
 #include <envDefs.h>
+#include <epicsString.h>
 #include <osiSock.h>
 
 #include <epicsUnitTest.h>
@@ -30,7 +31,59 @@ void setenv(char * a, char * b, int c)
 
 using namespace epics::pvAccess;
 using namespace epics::pvData;
-using namespace std;
+
+static const char indata[] =
+        "hello =    world  \n"
+        "  # oops\n"
+        " #dd=da\n"
+        " empty =  \n"
+        " this   =   is a test\n\n"
+        ;
+
+static const char expectdata[] =
+        "empty = \n"
+        "hello = world\n"
+        "this = is a test\n"
+        ;
+
+static
+void showEscaped(const char *msg, const std::string& s)
+{
+    std::vector<char> chars(epicsStrnEscapedFromRawSize(s.c_str(), s.size())+1);
+    epicsStrnEscapedFromRaw(&chars[0], chars.size(), s.c_str(), s.size());
+    testDiag("%s: '%s", msg, &chars[0]);
+}
+
+static
+void testProp()
+{
+    Properties plist;
+
+    {
+        std::istringstream input(indata);
+        plist.load(input);
+        plist.list();
+        testOk1(!input.bad());
+        testOk1(input.eof());
+    }
+
+    testOk1(plist.size()==3);
+    testOk1(plist.getProperty("hello")=="world");
+    testOk1(plist.getProperty("this")=="is a test");
+    testOk1(!plist.hasProperty("foobar"));
+
+    {
+        std::ostringstream output;
+        plist.store(output);
+        std::string expect(expectdata), actual(output.str());
+
+        testOk1(!output.bad());
+        testOk(expect.size()==actual.size(), "%u == %u", (unsigned)expect.size(), (unsigned)actual.size());
+        testOk1(actual==expectdata);
+        showEscaped("actual", actual);
+        showEscaped("expect", expect);
+    }
+}
 
 static void showEnv(const char *name)
 {
@@ -60,9 +113,9 @@ static void showAddr(const osiSockAddr& addr)
     } while(0)
 
 
-MAIN(configurationTest)
+static
+void testConfig()
 {
-    testPlan(35);
     testDiag("Default configuration");
     Configuration::shared_pointer configuration(new SystemConfigurationImpl());
 
@@ -126,7 +179,13 @@ MAIN(configurationTest)
 
     Configuration::shared_pointer configurationOut(configProvider->getConfiguration("conf1"));
     testOk1(configurationOut.get() == configuration.get());
+}
 
+MAIN(configurationTest)
+{
+    testPlan(44);
+    testProp();
+    testConfig();
     return testDone();
 }