Re: [Liborigin-devel] Bug fixes

[Knut F. <Knu...@gm...>]

Okay; since the patches are definitely an improvement and even include some 
comments explaining the weird behaviour, I won't let better be the enemy of 
good. Committed.

However, this bug raises a more general question to keep in mind for the 
generalized parser:

[2010-11-10 09:25] Miquel Garriga <gbm...@gm...>:
> 2010/11/10 Knut Franke <Knu...@gm...>:
> > What findStringPos() *should* do is guarantee that the read position does
> > not change (i.e. seekg(startPos) before any return) and clearly indicate
> > failure. There are different conventions for that (-1, a special value a
> > la std::string::npos, returning bool or error code and writing the
> > actual result to a pointer as offered by the GSL, throwing an
> > exception). I think it would help if we could try to stick to one such
> > convention.
> 
> Yes, you are right, but, since the generalized parser will not  need
> findStringPos()
> at all,

Maybe; but it will definitely need some kind of error handling for cases where 
a (possibly corrupted, or unsupported) file is not parsable by some (internal) 
function. Having a set convention would help avoid bugs like these, where 
findStringPos() returns the position of the last byte in the file in case of 
failure, while callers variously expect -1, expect the read position to be at 
EOF or ignore this case altogether.

Personally I think that exceptions would provide a good error-signalling 
convention. Maybe this particular example is a corner case, since very similar 
methods in the STL use the magic value std::string::npos.

> You should not return -1 in an
> unsigned int,

Of course, with that convention the return type needs to be changed.

> However, I would better spend time in the generalized parser that
> finding a good  patch
> for each bug in OriginNNNParser.

Fair enough.

Knut