Re: [PATCH] ed with perl-compatible regular expressions

[Martin Guy]

On 22/07/2021, Shawn Wagner <shawnw.mobile@gmail.com> wrote:
> Take 2, based on Martin's feedback. Now uses the locale to determine
> if you're working with UTF-8 or not (using the same logic as GNU
> grep's -P) instead of an explicit option.

Thanks. Of course, I don't know whether the GNU ed maintainer will
accept the patch, as on the one hand it's supposed to be a free
implementation of traditional ed, and on the other, "GNU extensions"
are a common phenomenon.
Anyway, here are some more detailed suggestions, to help it get in.

diff --git a/Makefile.in b/Makefile.in
-       $(CC) $(LDFLAGS) $(CFLAGS) -o $@ $(objs)
+       $(CC) $(CFLAGS) -o $@ $(objs) $(LDFLAGS)

This is more correct, true, but not part of the PCRE2 modifications.
To be super-correct, this should probably be submitted a a separate patch.

Similarly, the additional tests for -E mode should be submitted separately.

Instead of modifying the edE testsuites and adding a flag to check.sh,
is it feasible to provide edP tests in a separate file that is only
run if HAVE_PCRE2?

diff --git a/configure b/configure
+                echo "  --disable-pcre2       Do not compile in PCRE2
support even if detected."
...
+        --disable-pcre2)       test_pcre2=no ;;

The usual configure convention is "--without-foo"

diff --git a/main.c b/main.c
+          "  -P, --perl-regexp          use perl-style regular expressions\n"

I'd place this under #ifdef HAVE_PCRE2 so that the help message
matches what the compiled version does. Actually, me, I'd put all the
PCRE2-related code under #ifdefs to reduce binary size and
dependencies, e.g. on "wchar.h" and mbrtowc().
There is an argument that, if PCRE2 is not supported and someone uses
"ed -P", it should say that PCRE2 is not included in this version of
ed, but the current "ed: invalid option -- 'P'" is probably
sufficient.
By #ifdeffing you also ensure that, if PCRE2 support is not available,
the code that gets compiled is the same as it was before the patch,
which avoids any possible portability problems.

Is there a cleaner way to achieve that without redefining posix_re to
re_type, such as using a separate data structure when in PCRE2 mode?
That would avoid having to modify as much non-pcre code.

-    if( subst_regex_ ) regfree( subst_regex_ );
+    if( subst_regex_ )
+      {
+        if ( perl_regexp() )
+          {
+#ifdef HAVE_PCRE2
+          pcre2_code_free( subst_regex_->pcre.re );
+          pcre2_match_data_free( subst_regex_->pcre.md );
+          subst_regex_->pcre.re = 0; subst_regex_->pcre.md = 0;
+#endif
+          }
+          else regfree( &(subst_regex_->posix_re) );
+      }

It's usual to put the shorter if-then-else branch first for
code-reading clarity,
similarly for the other "if( perl_regexp() )" cases

Other than these points, it looks OK to me.

  M