StringOutputStream

[OrbWeaver]

Greebo -- I notice you have changed StringOutputStream to a typedef for std::ostringstream, which in theory is a good idea (i.e. using STL classes instead of homegrown rubbish). However I have encountered some errors.

 

1. Calls to std::ostringstream(int) constructor (corresponding to the "capacity" argument of StringOutputStream) do not work, since there is no such constructor.

2. std::ostringstream does not have a c_str() method, you have to use str() to get a string and then call c_str() on this.

3. StringOutputStream has overloaded operator<< functions for (stupid) custom classes like DirectoryCleaned, which do not work for ostringstream.

 

Assuming you didn't see these errors before committing, could this be a Win32/Linux issue? I can probably fix most of them, but I have no idea how many there are going to be.

[OrbWeaver]

Never mind, there weren't that many errors and I could fix all of them.

[greebo]

Sorry, didn't see your post in time. I'm still in the middle of the refactoring process and there is a lot of old code lying around, either unused or commented out. I intended to switch to Linux and fix the remaining issues (the sconscript will have to be updated as well), but I'm not that far yet.

[OrbWeaver]

On the subject of refactoring, I am confused by how the "in loop increment" in XMLRegistry::removeKeyObserver() could be necessary to avoid a crash, since after looking at the code several times the two expressions seem to be functionally identical:

 

if (i->second == observer {
_keyObservers.erase(i++);
}
else {
i++;
}

 

is the same as

 

if (i->second == observer {
_keyObservers.erase(i);
i++;
}
else {
i++;
}

 

which is the same as

 

if (i->second == observer {
_keyObservers.erase(i);
}
i++;

 

which is equivalent (although potentially slower) than

 

if (i->second == observer {
_keyObservers.erase(i);
}
++i;

 

which is the same as putting the ++i in the final part of the enclosing for loop.

 

Is this a Windows issue, or am I just missing something really obvious?

[greebo]

The VC++ implementation of the STL iterators raise an exception if you try to dereference/increase an invalid iterator, which is the case here. The erase(i) method invalidates the given iterator (the element being pointed at is no longer existing after the call) and it can no longer be used. The post-increment i++ makes sure that the iterator is incremented before it is becoming invalid.

 

I'm not an expert when it comes to STL (I haven't got around to read my copy of Effective STL yet), but I think this is ok and not a glitch of the MSVC implementation. I'm surprised that gcc doesn't throw an error here when executing that code. I guess it just increases the encapsulated pointer without asking whether it is invalidated or not.

 

(On a related note, the VC++ debug mode automatically has turned on the switch _HAS_ITERATOR_DEBUGGING, which is brutally picky when it comes to invalid iterators). It's not active in release builds (and it makes STL operations really slow in debug builds), but so far all of the issues raised by this setting were valid.)

[OrbWeaver]

Ah yes, you are right. The two pieces of code differ in the relative order in which the erase and increment are executed.

[greebo]

I'm mostly through with my small refactoring project.

 

TextFileOutputStream: replaced by std::ofstream

StringOutputStream: replaced by std::ostringstream

TextOutputStream: replaced by std::ostream

 

The advantage is that globalOutputStream() accepts all std::* types now - no more stupid c_str() calls when inserting a std::string to the output stream.

 

Moreover, I could redirect the std::cout/cerr streams to our console and logfile with a few lines of code, so the actual OS console window (in Windows or Linux) is not needed anymore.

 

Another thing: The Console and the LogFile classes derive from an abstract LogDevice, which can be attached to the LogWriter. With this approach, it's possible to attach any number of LogDevices to our logging system and the LogWriter itself doesn't need to check for the existence of any LogDevice - it just iterates over all registered classes and that's it.

 

I'm currently removing all references to the old StringOutputStream, etc. classes, which is taking a while as I'm recompiling all the code with each step, to be sure that I'm not missing anything.

[OrbWeaver]

Sounds like a good piece of legacy code disposal. To be honest I can't actually think of an instance where I have seen a StringOutputStream that was actually necessary in the first place -- most of the uses could be replaced with simple std::string appends.