[Box Backup-dev] COMMIT r445 - box/chris/win32/vc2005-compile-fixes/bin/bbackupquery

[Martin Ebourne]

On Sun, 2006-02-12 at 15:04 +0000, subversion@fluffy.co.uk wrote:
> Author: chris
> Date: 2006-02-12 15:04:37 +0000 (Sun, 12 Feb 2006)
> New Revision: 445
> 
> Modified:
>    box/chris/win32/vc2005-compile-fixes/bin/bbackupquery/BackupQueries.cpp
> Log:
> * BackupQueries.cpp
> - Use const_iterator on all platforms except MSVC, which doesn't like calling
>   erase() with a const_iterator as a parameter

Well it doesn't happen very often, but I believe MSVC is right here and
g++ is wrong. Specifically, it appears to be a bug in libstdc++:
set::iterator is actually typedef'd to the underlying container's
const_iterator. Thus it will happily take const_iterator to erase,
despite it being declared as only taking iterator. There's a note in the
libstdc++ header file (look in bits/stl_set.h) about the type being
wrong, so it's obviously a known issue they've got problems with.

The correct fix here is to use the correct non-const iterator for these
two variables called 'local'. It also won't lose any const safety
because these vars are only ever used for calling erase() anyway.

> +#ifdef _MSC_VER
> +		typedef std::set<std::string>::iterator string_set_iter_t;

I personally advocate using typedefs for all container uses thus making
it much easier to access the iterators and not end up with crazy types
throughout the code, and make it much easier to make changes later
(define everything in one place only), so I like the way you were headed
here.

I would typedef the set like so (named after use not underlying type):

typedef std::set<std::string> FilenameSet;

Then it's FilenameSet::iterator, which is a whole lot tidier. I think
it's a shame they didn't manage to make types work like statics in the
standard. ie. so you could do somelistvar.iterator just like you can do
someobjectvar.staticmethod() without actually knowing the type. I think
they didn't because it caused major problems with the parsers and
templates, but it would have massively improved stl usability.

> -		for(std::set<std::string>::iterator i = localFiles.begin(); i != localFiles.end(); ++i)
> +		for(string_set_iter_t i = localFiles.begin(); i != localFiles.end(); ++i)

I've no idea why you're changing the type of this one when it isn't even
used for calling erase(). Only the two iterators called 'local' appear
to be affected.

Cheers,

Martin.