Use weak symbol create_fallback_regex to separate the implementation using PCRE2 and std::regex #77
Conversation
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D75173102 |
larryliu0820
changed the title
Summary: As titled, in D74864339 I changed the tiktoken dependency of `//xplat/cria/utils:prompt` to be `tiktoken_lookahead` which increased the binary size. This change can be reverted since Cria is not using this feature yet. Reviewed By: derekxu Differential Revision: D75173102
facebook-github-bot
force-pushed
the
export-D75173102
branch
from
59e570c to
59e7a62
Compare
|
This pull request was exported from Phabricator. Differential Revision: D75173102 |
Summary: As titled, in D74864339 I changed the tiktoken dependency of `//xplat/cria/utils:prompt` to be `tiktoken_lookahead` which increased the binary size. This change can be reverted since Cria is not using this feature yet. Reviewed By: derekxu Differential Revision: D75173102
facebook-github-bot
force-pushed
the
export-D75173102
branch
from
59e7a62 to
2ef5c05
Compare
|
This pull request was exported from Phabricator. Differential Revision: D75173102 |
| * @param pattern The regex pattern to compile. | ||
| * @return A unique pointer to an IRegex-compatible object. | ||
| */ | ||
| Result<std::unique_ptr<IRegex>> create_regex(const std::string& pattern); |
There was a problem hiding this comment.
Why not just make this the weak symbol?
There was a problem hiding this comment.
Because we can't have RE2 in both weak implementation and strong implementation
There was a problem hiding this comment.
I mean just have create_regex weak implementation do re2, then strong implementation does re2 + fallback. Basically what you are doing now, seems a bit clearer to me
|
|
||
| /** | ||
| * @brief Creates a fallback regex instance. If no strong symbol defined, | ||
| * returns Error, otherwise uses PCRE2 and std::regex. |
There was a problem hiding this comment.
If this is meant to be extensible to other regex libs, would make sense to me to remove PCRE2 from the comment and rename regex_lookahead.cpp to regex_pcre2.cpp or something like that.
There was a problem hiding this comment.
lookahead = pcre2 + std::regex, right?
jackzhxng
left a comment
jackzhxng
left a comment
There was a problem hiding this comment.
to unblock internal regression
Declare and define a weak symbol
create_fallback_regex, the default implementation is returning an error.We then build a separate target
regex_lookaheadfor PCRE2 and std::regex, to support lookahead feature. This library can be optionally linked into the binary.