This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [PATCH 3/5] tests: Add run-readelf-unknown-self.sh test to check for unknown DWARF codes.
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Tue, 14 Aug 2012 22:12:33 +0200
- Subject: Re: [PATCH 3/5] tests: Add run-readelf-unknown-self.sh test to check for unknown DWARF codes.
On Tue, 2012-08-14 at 11:28 -0700, Roland McGrath wrote:
> > +# Make sure there are no unknown DWARF codes.
> > +testrun_on_self_nomatch "unknown_0x[0-9a-f]+|lo_user\+0x[0-9a-f]" ../src/readelf -N -w
>
> You'll have to make it smarter without the unknown_ prefix,
> but it's better to work harder to write a test than to make
> the output of a user tool uglier.
Lets see if we can make the tool output nicer first in the other patch.
I don't think printing unknown is ugly.
> > + for file in $self_test_files; do
> > + testrun $* $file | egrep -o "$pattern" | sort -u > self_matches.out \
>
> Indent by two spaces, not four. Use "$@" (with quotes), not $*.
Will fix indentation. Now I am confused about the "$@" vs $* though.
Earlier you said:
"$@" is special magic but $@ is just $* and it looks more normal
to use $* where you don't need "$@".
http://article.gmane.org/gmane.comp.sysutils.elfutils.devel/2279
So do I need "$@" here instead of $*? What is the magic?
> > + || :
>
> Why do you want to succeed if the pipeline fails?
> Anyway this really should do testrun separately so it can check
> the exit status of the tested command before it does grep.
I think this was just cargo cult from testrun_out () above.
But I don't know precisely what you mean by running testrun,
we also do need to capture the output. But I'll look into rephrasing the
function.
> > + if test -s self_matches.out; then
> > + echo "*** failure in $* $file"; cat self_matches.out; exit_status=1;
>
> Use separate lines rather than ;.
OK.
> > + # Only exit if something failed
> > + if test $exit_status != 0; then exit $exit_status; fi
>
> If you want to do this on one line, use:
> test $exit_status = 0 || exit $exit_status
Which is shorter and nicer.
Thanks,
Mark