[RESOLVED] map iterators

[2kaud]

I'm converting some c++ code from MS VS2003 to MS VS2013 and I'm come across a couple of lines of code re map iterators that now won't compile. Without changing the way the code works, can any guru offer a way of getting this code to compile with VS20103.

in a header file

...
class PROGLIBSPEC cOpts {
private:
	//Type defs for map
	typedef std::map<cStr, cStr> TOPT;
	typedef TOPT::const_iterator OPTITER;

	TOPT	optns;

	OPTITER	piter;

public:

	//Default constructor
	cOpts()  {
		piter = NULL;
		optns.clear();
	}
...

in a .cpp file

...
	//Has GetFirst been done first
	if (piter == NULL) {
		return (GetFirst(name));
	}
...

The problem is the compare/assignment with NULL which was allowed in VS2003 but isn't in VS2013. Any ideas? The code in .cpp is trying to ascertain whether an iterator has been set or not.

Please don't suggest changing NULL to optns.begin() as a later test is for this value!

[Philip Nicoletti]

I don't think there is a real nice method.

1) Before reading your "please" comment, I was going to suggest optns.end() [ set after the clear() ]

2) use a separate bool variable

3) and similar to #2, create an empty map of the same type, set piter equal to that containers end() and check against that.

[2kaud]

I was hoping there was some constant I haven't thought of that could be used - I even tried nullptr but that also gave the error. I had thought of 2) but not of 3). I'll think I'll go with 3) as that will be easier to fit into the existing code as there is over 100 of these type of classes! - and I can get a junior to do the same change to all these classes.

I've made the new empty map a const static member in the class and this works.

Thanks.

[monarch_dodra]

Please don't suggest changing NULL to optns.begin() as a later test is for this value!

Hum... I'd have suggested "optns.end()" instead... ?

Although if you do modify optns, then later, your piter might not compare with .end()

I'd have suggested simply "OPTITER()", but depending on implementation detail, this *could* compare true to "optns.begin()", if the map is empty. So I wouldn't really on magic values here. I would not chose 3 for that reason too mind you.

I'd go for #2. Though instead of having a separate boolean, you could make your iterator "null-able", via things like Boost.Optional. If you can't boost, than unique_ptr could be a "quick fix". It has the advantage of being certifiably correct, and are safe (AFAIK) in the sense that you can't accidentally "forget" to test.

[2kaud]

Thank you for your reply. I've implemented 3) as this required the least changes to the member functions. Testing has indicated that this works fine. I'm not keen on this solution and have flagged it as a migration issue. However currently the work is to undertake the migration from VS2003 to VS2013 with as little code change/logic change as possible. Once the migration has been completed and tested to confirm that the migrated system works the same as the previous, then additional work will be schedules to be undertaken to update and 'modernise' the code using the full range of c++11/14 features (much of this code was done using MS VC6). And before someone suggests, no we don't do a migration and updates at the same time - that way lies big headaches if things don't work properly afterwards!

[superbonzo]

Although if you do modify optns, then later, your piter might not compare with .end()

are you sure ? map does not invalidate iterators (unless previously erased), so end() is required to not get invalidated, AFAIK.
That said, I agree that boost:: optional would be the way to go ... ( unless piter is truly meant to mean end() ).

I've implemented 3) as this required the least changes to the member functions. Testing has indicated that this works fine. I'm not keen on this solution and have flagged it as a migration issue. However currently the work is to undertake the migration from VS2003 to VS2013 with as little code change/logic change as possible. Once the migration has been completed and tested to confirm that the migrated system works the same as the previous, then additional work will be schedules to be undertaken to update and 'modernise' the code using the full range of c++11/14 features

AFAIR, note that the solution of comparing with end() is valid for c++03 too.

[monarch_dodra]

Thank you for your reply. I've implemented 3) as this required the least changes to the member functions. Testing has indicated that this works fine. I'm not keen on this solution and have flagged it as a migration issue. However currently the work is to undertake the migration from VS2003 to VS2013 with as little code change/logic change as possible. Once the migration has been completed and tested to confirm that the migrated system works the same as the previous, then additional work will be schedules to be undertaken to update and 'modernise' the code using the full range of c++11/14 features (much of this code was done using MS VC6). And before someone suggests, no we don't do a migration and updates at the same time - that way lies big headaches if things don't work properly afterwards!

Sounds like a good plan actually

[Philip Nicoletti]

I would not chose 3 for that reason too mind you.

Could you explain this further ?

Are you saying:

SomeMapType m1,m2;

if (m1.end() == m2.end())  // this is possible
{
}

I don't think this is possible by the standard (but it looks like it was the case for Vs2003?). It seems like "iterator reachability" would preclude that possibility (24.2.1-6). But I could be wrong, the wording of much of the standard is over my head.

[superbonzo]

It seems like "iterator reachability" would preclude that possibility (24.2.1-6).

Reachability does not excluded that. For example, consider a graph with a tree topology; it could expose forward iterators from leaf to (the common) root, or even bidirectional iterators ( with some "branching" logic inside ). Note that the standard says that if i is reachable from j then they lie in the same "sequence", not the same "container" ...

that said, being map iterators bidirectional, i==j => --i == --j; so, unless I'm missing something, this along with the fact that end() never gets invalidates it should follows that m1.end() != m2.end() whenever m1 and m2 refer to different map instances.

[monarch_dodra]

are you sure ? map does not invalidate iterators (unless previously erased), so end() is required to not get invalidated, AFAIK.
That said, I agree that boost:: optional would be the way to go ... ( unless piter is truly meant to mean end() ).

Actually, that's a good point. I forgot about that. That means that my_map.end() is always valid *for a given map*, regardless of what you do to that map.

I would also guess that that kind of also guarantees that default initialization is also guaranteed to yield a unique (eg NULL) iterator? In this case, I don't think it even warrants messing with a static map, and one could just do "if ( piter == OPTITER() ) ..."

[2kaud]

In this case, I don't think it even warrants messing with a static map, and one could just do "if ( piter == OPTITER() ) ..."

That's a great idea and makes the change trivial - it even works too!

[superbonzo]

Actually, that's a good point. I forgot about that. That means that my_map.end() is always valid *for a given map*, regardless of what you do to that map.

I would also guess that that kind of also guarantees that default initialization is also guaranteed to yield a unique (eg NULL) iterator? In this case, I don't think it even warrants messing with a static map, and one could just do "if ( piter == OPTITER() ) ..."

uhm, yes and no ( well, to be really picky, I know ); default initialization gives you singular iterators, that may or may not compare equal ( consider "int *a,*b; assert(a==b); // we don't know" ); OTOH, value initialized forward iterators must give the same singular iterator, so that given "ForwardIter a{}, b{};" effectively implies a==b; but, this does not imply that a container iterator never compares equal to a value initialized iterator of the same type ( for instance, an empty container begin/end may simply return a pair of value-intialized iterators ); but yes, given that we know that map::end() do return a valid and unique iterator we can conclude that your conclusion is correct ( ie it's safe to assume that piter == OPTITER() works as expected )

... with the caveat that OPTITER() effectively gets value-initiazialized; unfortunately, some older compilers may not correctly value-initialize certain kind of objects.

[OReubens]

A lot has changed from VS2003 to 2013.
many containers not only have different parameters/return types on their members , but also behave differently.

You should consider proper code revisions rather than singling out individual lines of code and attempting to "fix" them.
Or stick with VS2013.

[jwbarton]

There is another problem with using the default initialization of the iterator which you won't notice until you run the code in DEBUG mode in VS2013.

The debug version of the iterator comparison includes a check for iterators being within the same container. If you try to compare iterators from 2 different maps in debug mode, the run-time will assert with a debug error "map/set iterators incompatible".

This includes iterators to elements inserted into the map, the iterator returned from .end(), and also includes default initialized iterators.

[2kaud]

A lot has changed from VS2003 to 2013.
many containers not only have different parameters/return types on their members , but also behave differently.

You should consider proper code revisions rather than singling out individual lines of code and attempting to "fix" them.
Or stick with VS2013.

Yes! The code now compiles 'cleanly' but of course this doesn't mean it still works. The code is now with testing to identify any conversion issues. Revision tickets have been raised where issues such as this have already been encountered and any further issues identified via testing will also raise revision tickets. Once all revision tickets have been examined then the way forward re the code base will be decided.

The way we are approaching conversion from old code bases to 'modern' c++11 is to firstly get the code to compile cleanly with VS2013 with as little code modification as possible, then to undertake testing with minimal code changes to get it work as previously. Then we undertake a code review to decide what/how needs changing. This approach has served us well previously.