Conversation
|
Hi! I think it's still possible to add optional support for dynamic allocation that is configured by preprocessor definitions ( |
|
thanks, you are right. |
|
Let's wait for some reply from the repo owner, I'm just a regular user here after all. I'll try to add a code review for your changes anyway, just in hope anyone finds it useful. |
re.c
Outdated
| /* Public functions: */ | ||
| int re_match(const char* pattern, const char* text, int* matchlength) | ||
| { | ||
| #if defined(RE_ENABLE_MULTI_PATTERNS) && (RE_ENABLE_MULTI_PATTERNS == 1) |
There was a problem hiding this comment.
I'm not sure about this part but in my understanding one don't have to check defined(RE_ENABLE_MULTI_PATTERNS) every time, just a single check in re.h (#ifndef ... #define ... #endif) seems to be enough and it's already there. At the same time I see that this construction is common for both this and other repos of the author, so your change is consistent with the other code. It's interesting what @kokke thinks about such checks.
re.c
Outdated
| #if defined(RE_ENABLE_MULTI_PATTERNS) && (RE_ENABLE_MULTI_PATTERNS == 1) | ||
| re_p = (re_t)calloc(1, sizeof(re_compiled)); | ||
| memcpy(re_p, re_compiled, sizeof(re_compiled)); | ||
| return (re_t)re_p; |
There was a problem hiding this comment.
re_p is already an instance of re_t, why would anyone need this cast? I'm also unsure about the similar cast below.
re.h
Outdated
| int re_match(const char* pattern, const char* text, int* matchlenght); | ||
| int re_match(const char *pattern, const char *text, int *matchlenght); |
There was a problem hiding this comment.
It's better to avoid mixing functional and style changes in one commit. This change also breaks style consistency with the rest of the code, doesn't it? Patches should preserve existing code style even if it's not your favorite one.
|
thanks, |
|
Btw, you can also squash commits and re-push to avoid unnecessary intermediate changes. Probably it's even possible to do it via Github UI, I'm not sure (command-line tools are good enough for me). I should also say that it's better to wait for repo owner's reply, his opinion about some questionable parts may be different. I.e. you don't have to change things immediately after getting the first reply, sometimes it's better to wait for more comments and only then decide what to do next. |
|
@kokke |
|
I think my PR might be an alternative to this: #58 |
good |
3 participants
Support for multiple patterns
When multiple patterns are used simultaneously, the existing code will fail，
the PR fix it. example：
int match_length;
const char* string_to_search = "ahem.. 'hello world !' ..";
re_t p1 = re_compile("[Hh]ello [Ww]orld\s*[!]?");
re_t p2 = re_compile("hello[0-9]");
re_t p3 = re_compile("fadsf*");
int match_idx;
match_idx = re_matchp(p1, string_to_search, &match_length);
match_idx = re_matchp(p2, string_to_search, &match_length);
match_idx = re_matchp(p3, string_to_search, &match_length);
re_freecompile(p1);
re_freecompile(p2);
re_freecompile(p3);