Replace UNIX regex API with Standard C++ regex library #97
Conversation
tobiolo
force-pushed
the
regex-stdcpp
branch
4 times, most recently
from
b68ee3c to
2280f2f
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the UNIX regex API with the Standard C++ regex library to improve portability and modernize the code.
- Removed platform-specific regex tests and updated them to use standard regex patterns.
- Replaced legacy <regex.h> with in the header file and updated the regex type.
- Modified the regex compilation and matching methods in the implementation file to use std::regex and associated APIs.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/rx.t.cpp | Updated regex tests to rely solely on standard regex patterns. |
| src/RX.h | Replaced <regex.h> with and updated the _regex member type. |
| src/RX.cpp | Converted legacy regex functions to std::regex API for compile/match. |
|
LGTM, I am all in favor of using standards. For Timewarrior, this is fine, it is not expecting regex strings on the command line. However, this may be an issue for Taskwarrior users? 👉🏻 @djmitche |
src/RX.cpp
Outdated
| std::string::const_iterator search_start(in.cbegin()); | ||
| std::string::const_iterator search_end(in.cend()); | ||
|
|
||
| while (std::regex_search(search_start, search_end, sm, _regex)) { |
There was a problem hiding this comment.
This could be simplified quite a bit!
There was a problem hiding this comment.
Thanks for your feedback! It can be written a lot nicer than this.
| ut.ok (r9.match (start, end, text), "e there are matches"); | ||
| ut.is (start.size (), (size_t) 6, "e == 6 matches"); | ||
|
|
||
| #if defined(DARWIN) || defined(CYGWIN) || defined(FREEBSD) || defined(OPENBSD) |
There was a problem hiding this comment.
Deleting a bunch of test cases doesn't fill me with confidence! It looks like, at least for these platforms, some of the allowed syntax changes. The details of the changes should certainly be documented and tested, and we can evaluate the impact on Taskwarrior from there.
In particular, if \d doesn't work to match a digit anymore, that might affect a lot of users.
There was a problem hiding this comment.
This issue seems to be solved, too. I just had to stick with ECMAScript grammar that supports all test cases.
There was a problem hiding this comment.
Awesome! In that case, is this change expected to have no user-visible impact?
There was a problem hiding this comment.
As far as I can tell from Ecmascript at cppreference.com there is no user-visible impact. The ECMAScript grammar seems to cover the use cases.
tobiolo
force-pushed
the
regex-stdcpp
branch
2 times, most recently
from
0804d06 to
d8ff51b
Compare
|
This would be ready to go from my side :-) Thanks for your reviews so far! |
3 participants
No description provided.