`split` regression between `1.13` and `1.14`

[toots]

There is a regression between 1.13 and 1.14 w.r.t. to split:

1.14:

# Re.Pcre.(split ~rex:(regexp "\\/") "///");;
- : string list = [""; ""]

1.13:

# Re.Pcre.(split ~rex:(regexp "\\/") "///");;
- : string list = [""; ""; ""; ""]

Looking at 8ec3918 this seems to be intentional.

Could you advise if this module intends on breaking the previous implementation starting with 1.14?

My preference would be to keep existing one so that I don't have to debug multiple regexp calls (and potentially our users).

But I understand that some times incorrect implementations have to be changed.

[rgrinberg]

I think the goal of Re.Pcre is to match the behavior of https://github.com/mmottl/pcre-ocaml . Differences in behavior are either bugs or are just due to limitations of our underlying re engine.

Could you check which implementation (1.13 or 1.14) is more faithful to Pcre.split?

[toots]

# Pcre.(split ~rex:(regexp "\\/") "///");;
- : string list = []

😳

I'm actually pretty surprised by this. Backward compatibility seems even trickier than I thought..

Here's the example that broke our automated tests:

# Pcre.(split ~rex:(regexp "\\/") "/path/to/file");;
- : string list = [""; "path"; "to"; "file"]

Here it is with re:

re 1.13:

# Re.Pcre.(split ~rex:(regexp "\\/") "/path/to/file");;
- : string list = [""; "path"; "to"; "file"]

re 1.14:

# Re.Pcre.(split ~rex:(regexp "\\/") "/path/to/file");;
- : string list = ["path"; "to"; "file"]

[rgrinberg]

Tbh, I think the version in 1.13 makes more sense. Because it at least satisfies String.concat ~sep:"/" split_components = original_string. @vouillon what do you think?

[toots]

My guess as a programmer would be that a reasonable expectation is that:

fun s -> String.concat "/" (Re.Pcre.(split ~rex:(regexp "\\/") s))

should be idempotent.

I never noticed the Pcre behavior with "///" returning [] and I don't know if it'd be a good idea to reproduce it but having split and concat be inverse of each other seems like a good rule.

[toots]

Tbh, I think the version in 1.13 makes more sense. Because it at least satisfies String.concat ~sep:"/" split_components = original_string. @vouillon what do you think?

Ha!