[Octave-patch-tracker] [patch #10292] Switch from PCRE to PCRE2

[Rafael Laboissière]

Follow-up Comment #5, patch #10292 (project octave):

Attached to this message is a revised patch. I addressed the issues raised by
John and Markus. Below are some comments:

[John]
> Octave's public header files should not depend on macros like
PCRE2_DATA_WIDTH, PCRE2_CODE_UNIT_WIDTH, or include the pcre2.h header
directly.  Using PCRE (or any library) is an implementation detail that
shouldn't be exposed to someone just wanting to use the regexp class.  That's
why we used the void pointer previously and only included the header files in
.cc files.
[Markus]
> * I see you moved including the header from the .cc file(s) to the .h file.
Would it be possible keep them in their old locations? That would reduce the
number of (unnecessary) symbols declared in user code. Or is there a reason
for this change?
I kept the scheme for including pcre2.h pretty similar to the previous one.
Now, there is no unnecessary symbols declared in the public header file
lo-regexp.h. I dropped the inclusion of pcre.h from regexp.cc, without
replacing it by the inclusion of pcre2.h, since this does not seem necessary.

[John]
> How do we know that a buffer size of 128 is sufficient for the call to
pcre2_get_error_message?  Should we try to adapt that in case the message ends
up being longer and pcre2_get_error_message returns a failure code?
I increased the buffer size to 256. This is what the upstream author does in
its demo program pcre2demo.c
<https://github.com/luvit/pcre2/blob/master/src/pcre2demo.c>. I guess that
this should be ok.

[John]
> If we need a change for OCTAVE_CHECK_LIB, that should probably be discussed
separately.
[Markus]
> Wrt changing OCTAVE_CHECK_LIB: It might be less intrusive to (temporarily)
add those defines to CPPFLAGS for this test.
The suggestion made by Markus is implemented in the new patch.

[Markus]
> * In lines 227 and following of your patch: I don't understand why `start`
and `end` are of type double to begin with. You changed it to type int.
Shouldn't it be of type std::size_t (or maybe better yet of type PCRE2_SIZE)
instead? I guess that would also apply to the respective properties of
`regexp::match_element`. Maybe that should be a separate change. But feel free
to include it here if you prefer.
IIUC, the variables `start` and `end` were defined as double in the original
code because they are passed as arguments `s` and `e` of
regexp::match_element, which are defined as double. I changed them to int in
order to simplify the code but, yes, it should be PCRE2_SIZE.

[Markus]
> * In lines 288 and 289: Static_casting between integer and floating-point
types might be risky for large values (that can be represented in one type but
not the other). Should there be more thorough checks? (That point might be
mute if we change the type of those properties and the corresponding
constructor in `regexp::match_element`.)
My preference is to let this for a separate change, if you agree.

[Markus]
> * In the code, you #define `PCRE2_DATA_WIDTH` and `PCRE2_CODE_UNIT_WIDTH`.
Should both also be defined for the configure check?
I think that I defined PCRE2_DATA_WIDTH because I have seen it in an PCRE2
example elsewhere. The code works without it. I dropped it from the patch.

[Markus]
> * Instead of repeating `pcre2_match_data_free` in several locations (which
could easily be missed in a future change), could you use `unwind_action`
instead? See, e.g., liboctave/util/oct-glob.cc:90:
> 
>       void *glob_info = octave_create_glob_info_struct ();
> 
>       unwind_action cleanup_glob_info_struct
>         ([=] () { octave_destroy_glob_info_struct (glob_info); });
> 
I implemented your suggestion.


(file #53948)

    _______________________________________________________

Additional Item Attachment:

File name: bug61542-v2.patch              Size:13 KB
    <https://file.savannah.gnu.org/file/bug61542-v2.patch?file_id=53948>



    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/patch/?10292>

_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/