PathMatcher Test : Demonstrate potential issue with find & Iterators.

[donboie]

C++ unit tests to demonstrate what I considered unexpected behaviour in PathMatcher.

Perhaps it's not worth keeping this code but it might be useful if we change functionality and it does document the current behaviour.

[johnhaddon]

Thanks Don. I did look at making the Iterator( RawIterator ) constructor explicit to make this trap harder to fall into. The problem with that was it made the really common for( Iterator it = p.begin().. idiom more painful. Thoughts? Should there be separate rawBegin() and begin() methods perhaps? I'm happy to take a look at this if you think that makes sense...

I've been moving away from using boost for the c++ unit tests, because having them split off elsewhere is awkward (I often forget to run the C++ tests), and boost unit_test has been a royal pain anyway. In Gaffer we just bind C++ tests in GafferTest etc so they can be called from the python tests. In Cortex this is a bit less clean because we don't have an IECoreTest module yet (we should do one day), but I've still adopted the same basic approach.

[donboie]

I did look at making the Iterator( RawIterator ) constructor explicit to make this trap harder to fall into. The problem with that was it made the really common for( Iterator it = p.begin().. idiom more painful. Thoughts? Should there be separate rawBegin() and begin() methods perhaps? I'm happy to take a look at this if you think that makes sense...

Perhaps an optional argument on PathMatcher::Iterator::Iterator which defines if it moves to the next explicit path or not? Good spot a few comments to explain what's going on also?

Let's just bin these changes, just wanted to see if there was any utility in keeping them.

[johnhaddon]

Perhaps an optional argument on PathMatcher::Iterator::Iterator which defines if it moves to the next explicit path or not? Good spot a few comments to explain what's going on also?

The whole purpose of the Iterator is to only visit explicit paths, so having an option to break that constraint seems wrong...

I take it you didn't like the begin()/rawBegin() idea? Any reason?

Let's just bin these changes, just wanted to see if there was any utility in keeping them.

I'd like to keep the PR open if that's OK. There are some definite traps here, and I do want to improve the situation somehow.

[donboie]

The whole purpose of the Iterator is to only visit explicit paths, so having an option to break that constraint seems wrong...

It doesn't break that constraint. It just constructs to ::end() when you attempt to construct on a non explicit path.

I take it you didn't like the begin()/rawBegin() idea? Any reason?

I like it but thought it didn't avoid the trap I ran in to, where used find to get an Iterator and didn't expect it move to the next explicit path on construction of a RawIterator. Perhaps if we also have a rawFind?

[johnhaddon]

It doesn't break that constraint. It just constructs to ::end() when you attempt to construct on a non explicit path.

Ah, I see what you mean. That sounds like an option. But then as things stand Iterator it = pm.begin() would always advance to the end unless the root is an explicit path. So I guess we would definitely need a rawBegin() and a magic begin() which constructed the Iterator internally.

I like it but thought it didn't avoid the trap I ran in to, where used find to get an Iterator and didn't expect it move to the next explicit path on construction of a RawIterator.

It would be in conjunction with making Iterator( RawIterator ) explicit. I think that would mean Iterator it = find() wouldn't compile, and then manually invoking the constructor might be enough to make you realise that some unwanted magic might be happening. Maybe that's not enough though?

[andrewkaufman]

Lets move this to an Issue (#851) rather than a PR.