-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fixes #23668; migrates from pcre to pcre2 #24405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devel
Are you sure you want to change the base?
Conversation
# passed from pcre_compile(). Those that are compatible with JIT execution are | ||
# flagged with J. | ||
|
||
PCRE2_MAJOR* = 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be the version of the dynamically loaded pcre2 library? the version on the OS might be anything, including much older ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to load a C define from a dynamic library?
#define PCRE2_MAJOR @PCRE2_MAJOR@
#define PCRE2_MINOR @PCRE2_MINOR@
#define PCRE2_PRERELEASE @PCRE2_PRERELEASE@
#define PCRE2_DATE @PCRE2_DATE@
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libraries usually have a function for that, to load it dynamically - even if you compile on one system, another system you run the compiled binary will have a different version making these defines entirely meaningless at best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ringabout the pcre2_config
API from pcre2 can help you get the version dynamically (at run time).
@arnetheduck I see the same hard-coding done in pcre.nim as well, see https://github.com/nim-lang/Nim/blob/devel/lib/wrappers/pcre.nim#L13
Why was that not a problem but hardcoding here is a problem? Is that because of the fact that there was no active development happening on pcre1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was that not a problem but hardcoding here is a problem?
It's a problem no matter the pcre version but this line of reasoning might not have been understood at the time of writing the pcre module either and indeed, the fact that pcre1 is dead contributed to the downstream issues not manifesting practically. The best outcome here is perhaps to deprecate these symbols (or indeed, deprecate the whole pcre module) because any code using them will have been "infected" by this gap in reasoning about what they represent.
fixes #23668
Most of
tre.nim
andtnre.nim
passed and didn't leak when checked with valgrind.TODO
Useful links
PCRE2Project/pcre2#51 (comment)
php/php-src#2857
i3/i3#4682 (comment)
https://copilot.microsoft.com