Raise exception Bad_option for unparseable or invalid options.

[yomimono]

Fix for issue #56.

[dbuenzli]

Disclaimer, I'm not familiar with this code base and/or design philosophy. However the patch in itself is a little bit strange as there's no way to catch the exception you defined except with a catch-all handler (which is bad idea) since you didn't declare it in the mli.

Where and who is supposed to handle the exception ? The new semantics of the function is quite different now (and it would be a good idea to document in the mli the fact unmarshal may raise).

[avsm]

Need to expose this in options.mli as well so that the exception can be caught.
It might also be worth putting a catch in for Failure (which is what the Cstruct.BE functions raise on a bounds overflow) and turning that into a Bad_option as well, so that the caller has an easier time of catching just the one error.

[avsm]

Yeah, agree with both those points (note that the mli was inadequate before @yomimono's patch too, as it didn't document that it might raise Failure).

On 3 Jul 2014, at 19:47, Daniel Bünzli notifications@github.com wrote:

Disclaimer, I'm not familiar with this code base and/or design philosophy. However the patch in itself is a little bit strange as there's no way to catch the exception you defined except with a catch-all handler (which is bad idea) since you didn't declare it in the mli.

Where and who is supposed to handle the exception ? The new semantics of the function is quite different now (and it would be a good idea to document in the mli the fact unmarshal may raise).

—
Reply to this email directly or view it on GitHub.

[dbuenzli]

@avsm Just by curiosity is there some general agreed upon (and documented of course) error handling strategy in mirage ?

[samoht]

@dbuenzli we use polymorphic variants, see the error types in https://github.com/samoht/mirage/blob/master/types/V1.mli. There is yet an intruder though https://github.com/samoht/mirage/blob/master/types/V1.mli#L835

[avsm]

My apologies -- it should be catching Invalid_argument and not Failure (I just noticed the error while fixing the cstruct docstrings in 1.3.0)

[dbuenzli]

Usually Invalid_argument is not supposed to be catched, it means programming error (e.g. String.get raises Invalid_argument, never rely on this behaviour as at a certain point you may want to compile with -unsafe in which case your code will segfault). The bound should somehow be checked at the right place.

[avsm]

I think it's fine to catch this from String.get -- if you compile with -unsafe then all bets are off with respect to type safety anyway. It's probably worth defining a new exception at some point, but Invalid_argument is what is currently raised and hard to revise now without a lot of auditing of the other cstruct users.

[dbuenzli]

It's not fine at all to catch Invalid_argument from String.get, -unsafe doesn't have anything to do with type safety, it only turns off array and string bound checking, for example if you have proved your bounds to be correct. It's still an opportunity you'd like to give yourself (e.g. if you want to make a head to head comparison with a C program you should compile with -unsafe: compare things that are comparable...)

[avsm]

I don't believe in using unsafe for this at all -- I prefer to explicitly use String.unsafe_get where appropriate (this could be easier now with GADTs supplying proof witnesses a little more easily than before). Either way, it's immaterial to this patch, since we're stuck with Invalid_argument in the short term.

[avsm]

It's also unclear to me how -unsafe interacts with the new Bytes module in 4.02.0. Need to investigate...

[dbuenzli]

I also do use String.unsafe_get in codecs but I'm always reluctant in doing so as it's not user overridable except by changing the sources. In some sense we'd like to have the converse of what we have now, i.e., a safe option that turns any String.unsafe_get into safe calls.

[avsm]

Yeah, I agree -- adding more safety via a command-line flag is ideal.