1.3.2 compilation error w/ g++4.1(ogrili2layer.cpp)

[jdjohnso@…]

std::list<T>::constterator has no assignment from or comparison to 0.

Will attach patch that should work for all C++ std library implementations

[jdjohnso@…]

fix std::list assignment & comparison against 0 (ogrili2layer.cpp)

[warmerdam]

Mateusz,

Could you review and apply this patch?  I can provide a test dataset if 
needed.

[Mateusz Łoskot]

John, I'd like to add a comment to one construction included in the patch.
Two lines are added to the OGRILI2Layer::GetNextFeature() function:

if (listFeatureIt == std::list<OGRFeature *>::const_iterator())
   listFeatureIt = listFeature.begin();

I'm not sure what is the expected behaviour of the comparison above
but it is not a safe construction.

Here is my explanation why I think so:
The iterator of std::list container is a bidirectional iterator which must follow all requirements of the forward iterator.
The C++ Standard document (Doc No: N1905=05-0165 from 2005-10-19) specifies following requirements regarding default constructor of forward (and so bidirectional) iterator:

24.1.1/Table 75

expression          assertion/note
                   pre/post-condition
----------------------------------------------------
X u; ---------- note: u might have a singular value.
                note: a destructor is assumed.
X()  ---------- note: X() might be singular.

Next, in the 2.4.1/5 the Standard says:

"Results of most expressions are undefined for singular values;"

So, the righ-hand-side of the expression above is assumed to have singular value, so the whole comparison is assumed to cause undefined behaviour.

I'm not sure my understanding of the purpose of this construction above is correct, but I'd change the body of the OGRILI2Layer::GetNextFeature() function as follows:

if (listFeatureIt != listFeature.end())
   return *(listFeatureIt++);
else // if the end is reached, reste to the begin
   listFeatureIt = listFeature.begin();

John, I'd like to hear your opinion if you don't mind.

[jdjohnso@…]

Your proposed solution changes the semantics of the function.
As previously written (before any of this patch stuff) the function would
return NULL when it gets to the end. The API user can then call the
ResetReading() function to start over. Your implementation will never retun
NULL and would therefore cause the caller to loop forever.

The entire purpose of this 'if it's not set, put it to the beginning' test is
so the API user doesn't have to call ResetReading() before calling
GetNextFeature() the first time. So we basically need a sentinal value to
identify this 'first time in' case. That's the only time we want to
automatically set the iterator to the beginning of the list.

I'm going back now to the C++ spec to read up more on 'singular' constructs. I
just can't image the result of that comparison being undefined.

[jdjohnso@…]

After reviewing the spec, I conceed your point. What a nasty little corner case. We can have a properly constructed object with an undefined value. :-(
Great catch! Shows the value of reviews, doesn't it?

Since there is no valid sentinal value for the iterator, we'll need a "sidecar" sentinal variable. I'll attach another patch in a bit.

[Mateusz Łoskot]

(In reply to comment #5)
> After reviewing the spec, I conceed your point. What a nasty little corner
> case. We can have a properly constructed object with an undefined value. :-(
> Great catch! Shows the value of reviews, doesn' it?

John, I don't agree.
It's like you can assign to index of 10-elements array value of 20
and the state of the index, in terms of the integer object state,
will be valid, but in terms of our array it won't.

> Since there is no valid sentinal value for the iterator,
> we'll need a "sidecar"
> sentinal variable. I'll attach another patch in a bit.

Thank you!

[jdjohnso@…]

2nd attempt at patch (no singular value usage)

[Mateusz Łoskot]

(In reply to comment #4)
> Your proposed solution changes the semantics of the function.
> As previously written (before any of this patch stuff) the function would
> return NULL when it gets to the end. The API user can then call the
> ResetReading() function to start over. Your implementation will never retun
> NULL and would therefore cause the caller to loop forever.

I understand.
Simply, I did not analysed the semantics of those ILI functions.
I just noticed suspiciouse semantics of single construcion, without the bigger
picture.
So, I may be certainly wrong.

> The entire purpose of this 'if it's not set, put it to the beginning' test is
> so the API user doesn't have to call ResetReading() before calling
> GetNextFeature() the first time.

I see, but I think ResetReading() may be required. User is also instructed to
call it before start readin in the OGR tutorial.

>  So we basically need a sentinal value to
> identify this 'first time in' case. That's the only time we want to
> automatically set the iterator to the beginning of the list.

Why not to default set listFeatureIt to listFeature.end() in the OGRILI2Layer
ctor?

This would mean: "we've not read anything yet".

[Mateusz Łoskot]

(In reply to comment #7)
> Created an attachment (id=388) [edit]
> 2nd attempt at patch (no singular value usage)
> 
> 2nd patch adds bIteratorValid member to the class. It's initialized to 0 and
> reset to 0 each time a push_back happens on the list. It's set to 1 in
> ResetReading() or when GetNextFeature() finds the iterator invalid.

I'll apply it soon.
Thank you.

[jdjohnso@…]

> I see, but I think ResetReading() may be required. User is also instructed to
> call it before start readin in the OGR tutorial.

That's good.

> Why not to default set listFeatureIt to listFeature.end() in the OGRILI2Layer
> ctor?
> 
> This would mean: "we've not read anything yet".

Because the interator returned by listFeature.end() becomes invalid with the first push_back(). If we were to compare against it later, the comparison results would be undefined. I think the separate bool is cleaner.

[jdjohnso@…]

Actually, if the spec requires calling ResetReading() before calling GetNextFeature, why have the 'not initialized yet' check in GetNextFeature at all? If the API user violates the API, he/she deserves a crash. :-)

The attempt to work around their improper use of the API just gives them licence to do it again. I think I've changed my opinion. i think we should remove the listFeatureIt initializer from the constructor and remove the 'not initialized' check from GetNextFeature.

[Mateusz Łoskot]

(In reply to comment #11)
> Actually, if the spec requires calling ResetReading() before calling
> GetNextFeature, why have the 'not initialized yet' check in GetNextFeature at
> all? If the API user violates the API, he/she deserves a crash. :-)

Yes, you're right.
May be Frank would want to add something to this discussion?

> The attempt to work around their improper use of the API just gives them
> licence to do it again. I think I've changed my opinion. i think we should
> remove the listFeatureIt initializer from the constructor and remove the 'not
> initialized' check from GetNextFeature.

John, does it mean new patch is on the way?

[jdjohnso@…]

3rd patch - just remove the code that deals with unitialized iterator

[Mateusz Łoskot]

> Here's the patch that just removes all the code trying to deal with an
> unitialized iterator.

John, I close this bug, in fact without applying any changes to ILI driver.

[jdjohnso@…]

Huh? Why close it (and mark it as fixed) without apply a fix?
Without any change, my initial problem of not being able to compile with gcc4.1 still exists.

(In reply to comment #14)
> > Here's the patch that just removes all the code trying to deal with an
> > unitialized iterator.
> 
> John, I close this bug, in fact without applying any changes to ILI driver.
> 

[Mateusz Łoskot]

I just revealed that John's fixes are already present in the CVS version of GDAL.
Incorrect initialization has been fixed with rev 1.5 (current) of ogrili2layer.cpp file.

Looking at dates, this bug was fixed (2006/06/06) after
GDAL 1.3.2 got out (May 2006).
So, everything should work in GDAL CVS.

NOTE: John's 2nd patch provides helpful tests of iterators validity, so it may be reasonable to apply those changes in future.

Just for archive, here is the difference between revisions 1.4 and 1.5:

mloskot:~/dev/gdal/_cvs/gdal$ cvs diff -r 1.4  ogr/ogrsf_frmts/ili/ogrili2layer.cpp
Index: ogr/ogrsf_frmts/ili/ogrili2layer.cpp
===================================================================
RCS file: /cvs/maptools/cvsroot/gdal/ogr/ogrsf_frmts/ili/ogrili2layer.cpp,v
retrieving revision 1.4
retrieving revision 1.5
diff -r1.4 -r1.5
2c2
<  * $Id: ogrili2layer.cpp,v 1.4 2005/11/21 14:56:31 pka Exp $
---
>  * $Id: ogrili2layer.cpp,v 1.5 2006/06/06 17:49:07 pka Exp $
30a31,33
>  * Revision 1.5  2006/06/06 17:49:07  pka
>  * STL compatibility (Bug 1178)
>  *
50c53
< CPL_CVSID("$Id: ogrili2layer.cpp,v 1.4 2005/11/21 14:56:31 pka Exp $");
---
> CPL_CVSID("$Id: ogrili2layer.cpp,v 1.5 2006/06/06 17:49:07 pka Exp $");
76d78
<     listFeatureIt = 0;
118a121
>     if (listFeature.size() == 1) ResetReading();
135d137
<     if (listFeatureIt == 0) listFeatureIt = listFeature.begin();

[Mateusz Łoskot]

I reopen this bug because John found new issue in changes between rev 1.4 -> 1.5 of ogrili2layer.cpp file.

Here is John's explanation posted to me directly:

///////////////////////////////////////////////////////////////////////
The patch you showed that has already been applied does indeed remove the lines causing the compilation errors.
However it also introduces a very questionable line in SetFeature.

The CVS version of SetFeature now looks like this:
--------------------
OGRErr OGRILI2Layer::SetFeature (OGRFeature *poFeature) {
    listFeature.push_back(poFeature);
    if (listFeature.size() == 1) ResetReading();
    return OGRERR_NONE;
}
--------------------

The line I'm concerned with is 'if (listFeature.size() == 1) ResetReading();'

This is still trying to work around the fact that people are calling GetNextFeature() w/o first calling ResetReading(). But the attempt is broken. A second call to SetFeature will do another push_back, potentially invalidating the listFeatureIt iterator.
If the users of the ili drivers have their hearts set on misusing the API, we should delete the 'if (listFeature.size() == 1) ResetReading();' line and apply my second patch. Otherwise we should just delete the line.
///////////////////////////////////////////////////////////////////////

Finally, I'm going to just delete the line pointed above.
In future, we will be able to apply John's 2nd patch with appropriate guards.

[Mateusz Łoskot]

This bug has been fixed in CVS.