Add RegexSet functionality to C API #288

tiehuis · 2016-10-19T05:40:02Z

These functions implement a C interface to the RegexSet api.

Some notes:

These do not include start offsets as the standard regex functions
do. The reason being is down to how these are implemented in the core
regex crate. The RegexSet api does not expose a public is_match_at
whilst the Regex api does. I would actually like to change this if possible
as the inconsistency this makes between rure and rure_set types is
not great.
This only tests a complete compile/match mainly for sanity. One or
two more tests targeting the specific areas would be preferred.
Set matches take a mutable array to fill with results. This is more
C-like and allows the caller to manage the memory on the stack if
they want.

Wanted to make a request before targeting these changes to clarify.

These functions implement a C interface to the RegexSet api. Some notes: * These do not include start offsets as the standard regex functions do. The reason being is down to how these are implemented in the core regex crate. The RegexSet api does not expose a public is_match_at whilst the Regex api does. * This only tests a complete compile/match mainly for sanity. One or two more tests targetting the specific areas would be preferred. * Set matches take a mutuable array to fill with results. This is more C-like and allows the caller to manage the memory on the stack if they want.

tiehuis · 2016-10-19T05:47:19Z

regex-capi/include/rure.h

@@ -165,7 +172,7 @@ rure *rure_compile(const uint8_t *pattern, size_t length,
 /*
 * rure_free frees the given compiled regular expression.
 *
- * This must be called at most once.
+ * This must be called at most once for any rure.
 */


This is slightly more clear I think. rure_set_free should match this.

BurntSushi · 2016-10-19T10:57:08Z

regex-capi/include/rure.h

+
+/*
+ * rure_set_matches compares each regex in the set against the haystack and
+ * returns an array of bools which correspond to if a match was found for


I think "to if a" -> "to a"

BurntSushi · 2016-10-19T10:58:02Z

regex-capi/include/rure.h

+ * caring which, use rure_set_is_match.
+ */
+bool rure_set_matches(rure_set *re, const uint8_t *haystack, size_t length,
+                      const bool *matches);


Should this be bool *matches instead of const bool *matches? (Since the data pointed to by matches will be modified.)

BurntSushi · 2016-10-19T11:00:00Z

regex-capi/src/rure.rs

+        unsafe { Box::from_raw(re as *mut RegexSet); }
+    }
+}
+


I think we probably want to expose RegexSet::len here so that callers can use it to allocate matches.

BurntSushi · 2016-10-19T11:04:56Z

These functions implement a C interface to the RegexSet api.

Thanks for doing this! At a first glance, this looks pretty good with a few nits.

So one thing that we should consider before merging this is how to incorporate rure_options into RegexSet compilation. We probably also need to think about flags as well.

The core problem you'll run into is that neither of these things is actually exposed in the regex crate proper.

Maybe we can come up with a straw man proposal for the C API and convince ourselves that it can be added later, so that we can merge this sooner.

These do not include start offsets as the standard regex functions
do. The reason being is down to how these are implemented in the core
regex crate. The RegexSet api does not expose a public is_match_at
whilst the Regex api does. I would actually like to change this if possible
as the inconsistency this makes between rure and rure_set types is
not great.

The inconsistency is in the regex crate as well.

The inconsistency exists for a reason. Getting the match offsets is hard.

This only tests a complete compile/match mainly for sanity. One or
two more tests targeting the specific areas would be preferred.

This is probably OK. Mostly, I think the C tests should just make sure that FFI is working. Testing the semantics of RegexSet is better suited for the regex crate proper. But I won't stop you from adding more tests. :-)

Set matches take a mutable array to fill with results. This is more
C-like and allows the caller to manage the memory on the stack if
they want.

Perfect.

tiehuis · 2016-10-20T06:49:05Z

So one thing that we should consider before merging this is how to incorporate rure_options into RegexSet compilation. We probably also need to think about flags as well.

The core problem you'll run into is that neither of these things is actually exposed in the regex crate proper.

Maybe we can come up with a straw man proposal for the C API and convince ourselves that it can be added later, so that we can merge this sooner.

I had a bit more of a look into RegexSet and would the following work for the actual crate implementation. Correct me if I've overlooked something:

Extend RegexBuilder with two new functions from_set and compile_set. from_set just takes a slice of patterns instead of a single. compile_set is similar but maps using From::RegexSet instead of From::Regex. This seems compatible since the types Regex and RegexSet just wrap an Exec type, with the bulk of the work already implemented within Exec.

New function names are just placeholders.

And actually, just that seems like it would be sufficient for the lack of options to me. This can easily be exposed to the C API. This would be a backwards compatible API change, so would adjust the versioning however you are doing it. A corresponding compile_must for the rure_set type may be of use then at this point, too.

The inconsistency is in the regex crate as well.

The inconsistency exists for a reason. Getting the match offsets is hard.

If that is the case and I can just omit the start argument from the match functions.

This is probably OK. Mostly, I think the C tests should just make sure that FFI is working. Testing the semantics of RegexSet is better suited for the regex crate proper. But I won't stop you from adding more tests. :-)

I'll just add a regex failure test for sanity.

Thanks for the quick response, let me know if the RegexBuilder modifications outline sounds fine and if so, I'll implement those changes hopefully tomorrow.

tiehuis · 2016-10-20T20:16:06Z

I noted previously that we would need some extra functions in the regex crate. This seems like an incompatible change however after reading the regex 1.0 RFC (at least in the short-term as you previously said). It looks like enough of the interface is public that we can just construct the RegexOptions ourselves and call use the ExecBuilder directly, though, so should still be feasible.

BurntSushi · 2016-10-20T23:51:06Z

Sorry I'm dragging, but the past few days have been action packed (including a trip to the animal ER). Using RegexOptions and ExecBuilder is regrettable (they are technically exposed, but not really part of the public API), but I'm OK with it as long as there is a straight-forward way to expand the C API.

These options do need to get exposed in the regex crate proper though too. Your from_set and compile_set idea is one I've tossed around too, but my problem with it is that if you build with from_set, then it'd be nice to not call compile. I guess we could just return an error instead of ruling it out statically.

To be clear, I'm happy merging this PR without these options in the C API, as long as there's a reasonable path to adding them in the future.

tiehuis · 2016-10-21T04:06:00Z

Not a problem.

I'd like to get something thought out in that would minimize any breaking future api changes. I'd also like to avoid modifying the regex crate if possible right now.

With these goals in mind, here is what I suggest.

Add options to the c api regex set by using RegexOptions directly. This would be an implementation detail and can be adjusted to use a proper interface when one lands in the core crate.
Add a dummy start variable to the rure_set match functions. This is supported internally but no public way of acessing this is present. This unused field can be updated when support lands in the core crate. For the moment, it would require callers of this to use a 0 value to ensure future compatibility.

This should be flexible enough for future improvements without any breaking api changes.

Also, as an aside regarding the RegexBuilder and its compatibility with RegexSet. The cleanest way I see is to just create a seperate builder for a RegexSet itself. That is a topic for another time though.

I'll be offline till Monday but that day (which is thankfully a public holiday for me) I'll implement these and see how it all fits.

BurntSushi · 2016-10-21T10:53:41Z

Sounds good. Two things:

You'll need to add RegexOptions to the internal module, which is in src/lib.rs.
You can implement the start functionality by using Exec and calling searcher.

In fact, I think both of these things means that you end up working around the use of RegexSet proper entirely.

tiehuis · 2016-10-25T06:48:23Z

Just a note that I've finished adding these features.

BurntSushi · 2016-11-08T00:24:54Z

@tiehuis Thank you!

tiehuis commented Oct 19, 2016

View reviewed changes

BurntSushi reviewed Oct 19, 2016

View reviewed changes

Update documentation + add rure_set_len function

1a71cfc

tiehuis added 5 commits October 24, 2016 14:04

Update prototypes with mock values

01644e0

Adjust rure to use internal regex methods where needed

8beff76

Add two more tests for options and match start sanity

d464edf

Update rure_set documentation for new fields

f54bd24

Reduce missing_doc attribute scope

50af527

BurntSushi merged commit 7968aad into rust-lang:master Nov 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add RegexSet functionality to C API #288

Add RegexSet functionality to C API #288

Uh oh!

tiehuis commented Oct 19, 2016

Uh oh!

tiehuis Oct 19, 2016

Uh oh!

BurntSushi Oct 19, 2016

Uh oh!

BurntSushi Oct 19, 2016

Uh oh!

BurntSushi Oct 19, 2016

Uh oh!

BurntSushi commented Oct 19, 2016

Uh oh!

tiehuis commented Oct 20, 2016 •

edited

Loading

Uh oh!

tiehuis commented Oct 20, 2016 •

edited

Loading

Uh oh!

BurntSushi commented Oct 20, 2016

Uh oh!

tiehuis commented Oct 21, 2016

Uh oh!

BurntSushi commented Oct 21, 2016

Uh oh!

tiehuis commented Oct 25, 2016

Uh oh!

BurntSushi commented Nov 8, 2016

Uh oh!

Uh oh!

Add RegexSet functionality to C API #288

Add RegexSet functionality to C API #288

Uh oh!

Conversation

tiehuis commented Oct 19, 2016

Uh oh!

tiehuis Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

BurntSushi Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

BurntSushi Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

BurntSushi Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

BurntSushi commented Oct 19, 2016

Uh oh!

tiehuis commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiehuis commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BurntSushi commented Oct 20, 2016

Uh oh!

tiehuis commented Oct 21, 2016

Uh oh!

BurntSushi commented Oct 21, 2016

Uh oh!

tiehuis commented Oct 25, 2016

Uh oh!

BurntSushi commented Nov 8, 2016

Uh oh!

Uh oh!

tiehuis commented Oct 20, 2016 •

edited

Loading

tiehuis commented Oct 20, 2016 •

edited

Loading