Re: [lmi] Best way to integrate PCRE

[Vadim Zeitlin]

On Wed, 28 Jul 2021 19:45:30 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 2021-07-28 18:45, Vadim Zeitlin wrote:
GC> > On Wed, 28 Jul 2021 16:10:34 +0000 Greg Chicares 
<gchicares@sbcglobal.net> wrote:
GC> 
GC> [...simply withdraw support for msw builds of 
'test_coding_rules{EXE_EXT}'...]
GC> 
GC> >  Note that this will also require making parts of makefiles conditional on
GC> > the target host, which wasn't the case so far. We can do this, of course,
GC> > but I just wanted to mention that this is going to make the makefiles
GC> > slightly more complex and less linear. I don't think you mind, as you
GC> > probably wouldn't have proposed doing this otherwise, but please let me
GC> > know if this consideration changes anything.
GC> 
GC> That doesn't change the decision, strategically. Tactically, I'd favor
GC> a minimalist approach along the lines of this untested patch:
GC> 
GC> --------8<--------8<--------8<--------8<--------8<--------
GC> diff --git a/workhorse.make b/workhorse.make
GC> index 783bde355..f0b4bf6d3 100644
GC> --- a/workhorse.make
GC> +++ b/workhorse.make
GC> @@ -120,7 +120,12 @@ ifeq (,$(USE_SO_ATTRIBUTES))
GC>      ihs_crc_comp$(EXEEXT) \
GC>      lmi_md5sum$(EXEEXT) \
GC>      rate_table_tool$(EXEEXT) \
GC> -    test_coding_rules$(EXEEXT) \
GC> +
GC> +  ifeq (x86_64-pc-linux-gnu,$(LMI_TRIPLET))
GC> +    default_targets += \
GC> +      test_coding_rules$(EXEEXT) \
GC> +
GC> +  endif
GC>  
GC>    ifneq (so_test,$(findstring so_test,$(build_type)))
GC>      default_targets += \
GC> -------->8-------->8-------->8-------->8-------->8--------

 Thanks for this idea! I've followed it initially but, after much
toing-and-froing, finally decided to use a different approach: I still
always build test_coding_rules${EXEEXT}, but it just outputs an error if
it's built under MSW.

 I believe this is the best solution because

- It results in more clear error under MSW if something attempts to run
  test_coding_rules there.

- It avoids any possibility of breaking something else because
  test_coding_rules executable is not available when it's supposed to be.

- It still allows to build the fully functional version under MSW by simply
  predefining LMI_HAS_PCRE if you do have PCRE library there.

but please let me know if you disagree before I submit my patch.


GC> We'd also need to change 'hooks/pre-commit', e.g. (untested):

 With my solution above this is not necessary any more as it would run and
just output an error under MSW.


GC> Oh, and we'll need something like this, too (untested):
GC> 
GC> --------8<--------8<--------8<--------8<--------8<--------
GC> diff --git a/nychthemeral_test.sh b/nychthemeral_test.sh

 But we indeed still need to change this file to skip the checks using
test_coding_rules under non-POSIX platforms.


 Finally, the real reason for writing this email, is that I've realized I
forgot to ask another question: what do you think should be done with
regex_test.cpp? In my current version, it was changed to use std::regex
instead of boost::regex and while I could just leave it like this, I don't
think it makes much sense to have unit tests for the functionality provided
by the standard library. And while it did make sense to compare Boost.Regex
performance with different combinations of "m" and "s" modifiers, such
modifiers are not supported by std::regex, so these tests don't make much
sense neither. The only way to approximate these modifiers that I found was
to use basic regex syntax (as opposed to the ECMA script default one), but
it doesn't seem really useful to compare them as we're almost certainly not
going to use the basic syntax for anything. OTOH it could be useful to
compare std::regex performance with PCRE2, but this would require adding
tests for LMI_HAS_PCRE to this test as well and I don't know if it's really
worth it.

 My personal preference would actually be to just remove this test
completely because I don't think it's very useful, but I guess you
would disagree with this, as otherwise you probably wouldn't have added it.
But if we keep it, should we use std::regex in the "unit test" part of it,
or PCRE? And what should we be comparing in the "benchmark" part of the
test?

 Please let me know if you have any preference or if you think we can just
leave this test be with the minimum changes required to make it compile
with std::regex instead of boost::regex and maybe improve (or remove) it
later.

 Thanks in advance!
VZ

pgpGlv6n4VE1l.pgp
Description: PGP signature