Two replies follow...
On Wed, 2007 Feb 21 10:14:38 +0000, Philip Hazel wrote:
>
> Comments on your message:
>
> > * Tweaked the handling of pcre.h, pcre_stringpiece.h and pcrecpparg.h so
> > that they don't get distributed. (These three, as you recall, are
> > generated from *.h.in files)
>
> Er, I think it was your suggestion that the best way to deal with
> version numbering for enviroments that have to compile "by hand" was to
> distribute pcre.h, wasn't it? I think this one needs to be reversed,
> probably with a prominent comment somewhere to say why.
Yes, very true. This has been addressed, comment and all.
> > * Make this use "/usr/bin/env perl" instead of "/usr/bin/perl" for
> > portability.
>
> How is "/usr/bin/env perl" different from just "perl"?
The former can be used in a #! line. It's a common idiom when you want the
interpreter to be found in $PATH, instead of hard-coding the location.
(Consider the poor fellow with /usr/local/bin/perl...)
> > * Could we rename this to perltest.pl? I'd like an explicit extension to
> > make clear that this is not a binary.
>
> I don't mind either way, but RunTest and RunGrepTest don't have
> extensions.
Those are less significant for me because of the funny names. "perltest",
however, looks like a binary at first glance.
(We could add .sh extensions to Run*Test if consistency is desirable...)
> Comments as a result of trying the patch:
>
> $ patch -p1 <../pcre-7.0-drg1.patch
> patching file autogen.sh
> patching file CMakeLists.txt
> patching file Makefile.am
> patching file RunGrepTest.in
> patch: **** malformed patch at line 371: jkl" $testdata/grepinputx >>testtry
>
> Oops. It's the pesky newline and CR characters that are breaking it. I
> removed the relevant lines from the patch and applied them later by
> hand.
That's odd---the patch should apply cleanly. (It certainly does on my end.)
> * When I compiled:
>
> pcre_compile.c:85:6: warning: "EBCDIC" is not defined
> pcre_compile.c:265:6: warning: "EBCDIC" is not defined
> pcre_compile.c:424:6: warning: "EBCDIC" is not defined
> pcre_compile.c:565:6: warning: "EBCDIC" is not defined
> pcre_compile.c:592:6: warning: "EBCDIC" is not defined
> pcre_compile.c:614:6: warning: "EBCDIC" is not defined
>
> This is because you have changed from "#define EBCDIC 0" to "#undef EBCDIC"
> without changing the source.
Ah, my bad. I did intend to leave those sorts of changes for later, but as
this is pretty minor, we may as well handle it now.
> Then there were several of these:
>
> In file included from pcrecpp.cc:33:
> ./config.h:209:1: warning: "_GNU_SOURCE" redefined
> <command line>:1:1: warning: this is the location of the previous definition
Yes; that's because some files are #including pcre_internal.h, which in
turn #includes config.h, when each *.c file should be #including config.h
directly. I also left this for later.
I'll comment out the _GNU_SOURCE bit for now.
> Nevertheless it compiled successfully and ran the tests OK.
>
> BUT: --disable-cpp did not work; it seems to be completely ignored. I
> tried a few other options such as the UTF8 ones, and they worked fine.
Yes, I found and fixed the problem with --disable-cpp.
> Thanks for your work on this patch. It seems to be getting better and
> better.
That's the idea :-)
On Wed, 2007 Feb 21 12:09:15 -0800, Craig Silverstein wrote:
>
> } * Tweaked the handling of pcre.h, pcre_stringpiece.h and
> } pcrecpparg.h so that they don't get distributed. (These three, as
> } you recall, are generated from *.h.in files)
>
> I agree with Philip that you should distribute pcre.h, even though
> it's auto-generated.
>
> Also, could you put a comment before the nodist_includes_HEADERS lines
> that explains why you're not distributing them? (Because you're
> distributing the foo.h.in versions instead, right?) Otherwise it can
> be confusing why some header files are distributed and some aren't.
Yes, yes and yes; it is done.
> } if WITH_PCRE_CPP
> } dist_html_DATA += doc/html/pcrecpp.html
> } endif
>
> For pcrecpp.3, you had an 'else' with this if statement, that put this
> in dist_noinst_FOO. Do you want to do the same thing here? I'm not
> sure what the else is for, so it's hard for me to say.
Yes, I hadn't really tested the no-C++ case. I've redone both those sets of
conditionals, and they're looking better (and working properly) now.
> -- RunGrepTest.in:
>
> I'd prefer we not use printf(1) if possible -- I don't know how
> standard it is. (I don't think bsd machines have it, for instance.)
> Perhaps use /bin/echo -e instead?
I don't know of any Unix environment that doesn't have printf(1)---and
"echo -e" is waaaay less portable. (E.g. Solaris echo(1) doesn't have the
-e option.)
If worse comes to worst, we could always put the pattern in a separate
file, and use e.g.
pattern=`cat testdata/foo`
but I think we'd rather avoid that....
> -- RunTest.in:
>
> } -if [ "@UTF8@" = "" ] ; then
> } +if [ "@UTF8@" = "no" ] ; then
>
> Is this definitely always the right way to go? What is @UTF8@ going
> to be in each of the following cases?
> ./configure --enable-utf8
> ./configure --disable-utf8
> ./configure
>
> Will it ever be the empty string?
Don't forget, once RunTest/RunGrepTest make use of "pcretest -C", these
substitutions will go away. But to answer your question: "yes", "no", "no".
> -- configure.ac:
>
> } # This facilitates -ansi builds under Linux
> } AC_DEFINE([_GNU_SOURCE], [], [Enable GNU extensions in glibc])
>
> If this is giving Philip problems, I suggest we comment it out for
> now. I don't think -ansi support needs to be a priority for this
> patch. We're making a lot of changes, so it's good to be conservative
> where we can afford to be!
Commented out for now.
> } if test "x$enable_shared" = "xno" ; then
> } AC_DEFINE([PCRE_STATIC],[1],[to link statically])
> } fi
>
> Indentation nit: should be two spaces. :-)
Yep!
> And maybe put the description on the second line, even though it fits
> on one? That way this is consistent with all the other AC_DEFINEs.
Eh... I made the description a little longer, too. The PCRE_STATIC bit will
eventually need to be jettisoned anyway, however.
> } # This must be last; it determines what files are written as well as config.h
>
> Alas, it's no longer last, so maybe remove this comment?
May as well! It's outta there.
> Overall looks good to me.
Great! I'll have a -drg2 patch ready shortly.
--Daniel
--
NAME = Daniel Richard G. ## Remember, skunks _\|/_ meef?
EMAIL1 = skunk@??? ## don't smell bad--- (/o|o\) /
EMAIL2 = skunk@??? ## it's the people who < (^),>
WWW = http://www.******.org/ ## annoy them that do! / \
--
(****** = site not yet online)
