Subject: Re: bug#32194: [PATCH] Use Gnulib regex for lib-src

From: Eli Zaretskii <[email-masked]>
Date: Sat, 04 Aug 2018 18:41:38 +0300

> From: Paul Eggert <[email-masked]>
> Cc: [email-masked]
> Date: Tue, 31 Jul 2018 11:21:38 -0700
> 
> Eli Zaretskii wrote:
> 
> > would it be possible to let the configure
> >     script also check -lregex for those functions?  This could be
> >     important on non-glibc systems.
> 
> It would no doubt be possible. However, we haven't found a need in other 
> applications that use Gnulib. Gnulib code tends to be more up-to-date 
> than system-supplied libraries, and in practice it's OK if the system 
> puts the regex code in a place where 'configure' won't find it, as Emacs 
> will just fall back on the better Gnulib version in that case.

The idea is that the system-supplied regex library is also GNU regex,
but one tuned better to the specific platform.  The configure script
could check whether that library is up to speed, before using it.

Obviously, not a terribly important issue, but I've seen projects that
do this.

> > 2. Do we really need to rename src/regex.c?  We could have 2 regex.c in
> >     separate directories, couldn't we?  If possible, I'd prefer not to
> >     rename files, as that makes VCS forensics more complicated.
> 
> Renaming src/regex.h works around problems with having two include files 
> src/regex.h and lib/regex.h that fight each other. I tried not renaming 
> src/regex.h at first, and then ran into problems and gave up; although I 
> surely could get it to work eventually, I figured that we'd continue to 
> have ongoing problems and so it would be better to bite the bullet.

Too bad, but I guess we have no choice.  I'd only ask that the
renaming be done as a separate commit before the rest of the changes
in those two files, so that all the changes in src/regex.[ch] will be
after the rename, as that will make future forensics easier.

> > 3. Is it really necessary to pull mbrtowc, locale_charset,
> >     nl_langinfo, and their dependencies?
> 
> I didn't know, so in my first cut I did the easiest thing and pulled 
> them all in. It's pretty easy to omit them; we can always add some back 
> if we find they're needed after all. Revised patch attached; it is the 
> first patch in the attached series. (The later patches are some of the 
> simplifications mentioned above.)

OK, thanks.

> > P.S. Is it true that this version will no longer support a build with
> > WIDE_CHAR_SUPPORT undefined, i.e. those which have only the C locale?
> 
> I don't see any issues with such a build. What sort of problem do you 
> have in mind?

AFAIU, you suggest removing the !WIDE_CHAR_SUPPORT code, but we
previously supported platforms that don't have all the prerequisites
for using that code.  So platforms which don't have WIDE_CHAR_SUPPORT
will now be required to provide it, or will fail to compile/run.  Am I
wrong?

> OK, revised patchset attached. This keeps the debugging code in question. I took 
> the liberty of renaming DEBUG (which clashes with other DEBUGs) to 
> EMACS_REGEX_DEBUG.

Fine with me.

> (malloc, realloc, free): Do not redefine.  Adjust all callers
> to use xmalloc, xrealloc, xfree instead.

Is this wise?  xmalloc etc. are tuned to allocate Lisp data and
aligned objects, something that isn't necessarily needed in regex
library.  Why not use the normal memory allocation routines?

Thanks.

P.S. I think we should compare the performance of etags before and
after the switch, just to be sure we aren't going to suffer a
performance penalty.