BatString.ml should use unsafe versions of set and get

[murmour]

This gives a slight performance boost to one of my programs.

[UnixJunkie]

Is it always guaranteed that i is in [0..n-1]?
This will need to be carefully reviewed by several maintainers I think.

[murmour]

Is it always guaranteed that i is in [0..n-1]?

I have carefully reviewed the changes again, and believe so, yes.

[gasche]

In 266d5f6 I started annotating occurrences of unsafe_{get,set} with static assertions in comments, to check that the invariants are indeed respected. I think we should consider completing this annotation work before merging the PR, but I have other things to do for the rest of the day. Feel free to help!

(In OCaml, assert is not disabled in builds, so using assert for static conditions is not a good way to remove array-access checks. It would be nice if we could use something like Why3 from OCaml to check those facts statically.)

[UnixJunkie]

I'll try to read the full diff tomorrow.
I need a fresh head.
I am worried it could insert security problems.

[murmour]

@gasche

In 266d5f6 I started annotating occurrences of unsafe_{get,set} with static assertions in comments, to check that the invariants are indeed respected. I think we should consider completing this annotation work before merging the PR, but I have other things to do for the rest of the day. Feel free to help!

A great idea!

I've added assertions to those functions that are affected by the PR (except a couple of really trivial cases), so that we could be more sure that the change does not cause regressions.

[gasche]

I don't trust the Enum module enough to ensure this property (or have you went to annotate all Enum functions involved here with static assertions to "prove" this as well?). You don't risk undefined behavior on trusting another module's interface specifications. I that you must use safe accessors here -- in any case, the overhead of Enum largely drowns out any bound access cost.

[murmour]

Your arguments are convincing. I've reverted this change.

[UnixJunkie]

'len-1' should be written 'len - 1'.
There are several changes like this, this is not the standard OCaml convention.

[UnixJunkie]

Appart for lines 405..410 and 732 (indentation), I am OK with the changes.
It would have been easier to review if only unsafe_get was introduced, not unsage_get plus
some indentation changes.
Sorry for sounding grumpy, but this is a useful contribution so thank you.

[murmour]

I've reverted a few unnecessary changes.

[murmour]

The branch was rebased to resolve conflicts. Are there any outstanding issues remaining?

[gasche]

Thanks for the ping, merged.

(For the record, I have mixed feelings about using unsafe operations, but I believe we have used due diligence here to avoid problems.)