* [PATCH] MIPS: Enable NewABI tests for SDE targets
@ 2012-08-01 14:43 Maciej W. Rozycki
2012-08-01 18:20 ` Maciej W. Rozycki
2012-08-02 18:39 ` Richard Sandiford
0 siblings, 2 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2012-08-01 14:43 UTC (permalink / raw)
To: Richard Sandiford, Alan Modra; +Cc: binutils
[-- Attachment #1: Type: text/plain, Size: 4489 bytes --]
Hi,
As easily observed in config.bfd SDE targets support NewABI these days as
well and therefore need testing. The change below enables NewABI tests
for GAS and LD.
Regrettably this turned out not as straightforward as it would seem,
especially for LD, as there is assumption about The NewABI Target being
Linux made in a few places. The change therefore makes additional changes
to make this assumption go away. As a side effect some IRIX failures have
gone away.
These additional changes required updates to the LD version of
run_dump_test -- I carried over features added to the GAS version of same,
specifically the optional EXTRA_OPTIONS argument and the "dump" keyword.
Then mips-elf.exp makes use of these changes to abstract the handling of
GAS and LD options across targets.
In the course of developing these changes the "dump" keyword turned out
unneeded, but I propose to keep it for consistency between the two
implementations of run_dump_test, especially as it looks reasonable to use
by targets whose maintainers prefer to bulk-run dump tests through a glob
pattern -- that obviously precludes calling the same test with
EXTRA_OPTIONS depending on configuration as I do here.
The rest of the changes are the usual padding/alignment fixes as
bare-iron MIPS targets align known sections to 4 while other MIPS targets
use 8.
I have regression-tested this using Alan's clever script (thanks, Alan!),
although with more interesting MIPS targets added (these included
mips-freebsd, mips64el-freebsd, mips-sgi-irix5 and mips-sgi-irix6 in
particular as these use incompatible LD emulations). This produced no
regressions and the following progressions:
mips-sgi-irix6 -FAIL: MIPS ELF got reloc n32
mips-sgi-irix6 -FAIL: MIPS ELF xgot reloc n32
mips-sgi-irix6 -FAIL: MIPS ELF got reloc n64
mips-sgi-irix6 -FAIL: MIPS ELF xgot reloc n64
mips-sgi-irix6 -ERROR: -mips32r2 .../ld/testsuite/ld-mips-elf/mips16-call-global-1.s: assembly failed
mips-sgi-irix6 -ERROR: -mips32r2 .../ld/testsuite/ld-mips-elf/mips16-intermix-1.s: assembly failed
Alan, while at it -- may I suggest that you add UNRESOLVED to the list of
test results watched? According to DejaGNU/POSIX test suite standard
documentation that test result means there is a problem with the test case
itself and such should not be missed. I've seen it happen where an
exception was thrown by the TCL interpreter for example because a
reference to an uninitialised variable was made and the appearance (or
removal) of such problems should be highlighted when regression testing.
I have such change applied to my copy of your script already.
2012-08-01 Maciej W. Rozycki <macro@codesourcery.com>
gas/testsuite/
* gas/mips/mips.exp: Set has_newabi for mips*-sde-elf* too.
* gas/mips/elf-rel-got-n32.s: Adjust padding.
* gas/mips/elf-rel-got-n64.s: Likewise.
* gas/mips/elf-rel23.s: Likewise.
* gas/mips/elf-rel28.s: Likewise.
* gas/mips/n32-consec.s: Likewise.
* gas/mips/elf-rel-xgot-n32.d: Adjust output expected.
* gas/mips/elf-rel-xgot-n64.d: Likewise.
* gas/mips/elf-rel23.d: Likewise.
* gas/mips/elf-rel23a.d: Likewise.
* gas/mips/elf-rel23b.d: Likewise.
* gas/mips/elf-rel28-n32.d: Likewise.
ld/testsuite/
* ld-mips-elf/emit-relocs-1a.s: Make section alignment uniform
across targets.
* ld-mips-elf/emit-relocs-1b.s: Likewise.
* ld-mips-elf/jalbal.s: Adjust padding.
* ld-mips-elf/elf-rel-got-n32-embed.d: New test.
* ld-mips-elf/elf-rel-got-n64-embed.d: New test.
* ld-mips-elf/elf-rel-xgot-n32-embed.d: New test.
* ld-mips-elf/elf-rel-xgot-n64-embed.d: New test.
* ld-mips-elf/elf-rel-got-n32.d: Remove -melf32btsmipn32.
* ld-mips-elf/elf-rel-got-n64.d: Remove -melf64btsmip. Adjust
output.
* ld-mips-elf/elf-rel-got-n64-linux.d: Remove -melf64btsmip.
* ld-mips-elf/elf-rel-xgot-n32.d: Remove -melf32btsmipn32.
Adjust output.
* ld-mips-elf/elf-rel-xgot-n64.d: Remove -melf64btsmip. Adjust
output.
* ld-mips-elf/elf-rel-xgot-n64-linux.d: Likewise.
* ld-mips-elf/reloc-1-n64.d: Remove -melf64btsmip.
* ld-mips-elf/mips-elf.exp: Set has_newabi for mips*-sde-elf*
too. Move tool flags from o32_as_flags and o32_ld_flags
variables into abi_asflags and abi_ldflags arrays. Adjust test
cases run to use them. Run the new tests.
* lib/ld-lib.exp (run_dump_test): Implement the EXTRA_OPTIONS
argument and the "dump" keyword.
OK to apply?
Maciej
binutils-mips-sde-test-newabi.diff
[Attached compressed due to its size.]
[-- Attachment #2: Type: application/octet-stream, Size: 16797 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MIPS: Enable NewABI tests for SDE targets
2012-08-01 14:43 [PATCH] MIPS: Enable NewABI tests for SDE targets Maciej W. Rozycki
@ 2012-08-01 18:20 ` Maciej W. Rozycki
2012-08-02 18:39 ` Richard Sandiford
1 sibling, 0 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2012-08-01 18:20 UTC (permalink / raw)
To: Richard Sandiford, Alan Modra; +Cc: binutils
Hi,
> gas/testsuite/
> * gas/mips/mips.exp: Set has_newabi for mips*-sde-elf* too.
> * gas/mips/elf-rel-got-n32.s: Adjust padding.
> * gas/mips/elf-rel-got-n64.s: Likewise.
> * gas/mips/elf-rel23.s: Likewise.
> * gas/mips/elf-rel28.s: Likewise.
> * gas/mips/n32-consec.s: Likewise.
> * gas/mips/elf-rel-xgot-n32.d: Adjust output expected.
> * gas/mips/elf-rel-xgot-n64.d: Likewise.
> * gas/mips/elf-rel23.d: Likewise.
> * gas/mips/elf-rel23a.d: Likewise.
> * gas/mips/elf-rel23b.d: Likewise.
> * gas/mips/elf-rel28-n32.d: Likewise.
>
> ld/testsuite/
> * ld-mips-elf/emit-relocs-1a.s: Make section alignment uniform
> across targets.
> * ld-mips-elf/emit-relocs-1b.s: Likewise.
> * ld-mips-elf/jalbal.s: Adjust padding.
> * ld-mips-elf/elf-rel-got-n32-embed.d: New test.
> * ld-mips-elf/elf-rel-got-n64-embed.d: New test.
> * ld-mips-elf/elf-rel-xgot-n32-embed.d: New test.
> * ld-mips-elf/elf-rel-xgot-n64-embed.d: New test.
> * ld-mips-elf/elf-rel-got-n32.d: Remove -melf32btsmipn32.
> * ld-mips-elf/elf-rel-got-n64.d: Remove -melf64btsmip. Adjust
> output.
> * ld-mips-elf/elf-rel-got-n64-linux.d: Remove -melf64btsmip.
> * ld-mips-elf/elf-rel-xgot-n32.d: Remove -melf32btsmipn32.
> Adjust output.
> * ld-mips-elf/elf-rel-xgot-n64.d: Remove -melf64btsmip. Adjust
> output.
> * ld-mips-elf/elf-rel-xgot-n64-linux.d: Likewise.
> * ld-mips-elf/reloc-1-n64.d: Remove -melf64btsmip.
> * ld-mips-elf/mips-elf.exp: Set has_newabi for mips*-sde-elf*
> too. Move tool flags from o32_as_flags and o32_ld_flags
> variables into abi_asflags and abi_ldflags arrays. Adjust test
> cases run to use them. Run the new tests.
> * lib/ld-lib.exp (run_dump_test): Implement the EXTRA_OPTIONS
> argument and the "dump" keyword.
Regrettably the next change I was about to submit uncovered a couple of
regressions affecting little-endian targets. They are fixed with the
update below. No further issues found. I have further widened MIPS
target coverage now.
For the record -- I'll be testing these MIPS targets with submissions
from now on: mips-ecoff mips-elf mips-sde-elf mips-sgi-irix5
mips-sgi-irix6 mips-freebsd mips-linux mips64-freebsd mips64-linux
mips64el-freebsd mips64el-linux mipsel-ecoff mipsel-elf mipsel-freebsd
mipsel-linux mipsisa32-elf mipsisa32-linux mipsisa32el-elf
mipsisa32el-linux mipsisa64-elf mipsisa64-linux mipsisa64el-elf
mipsisa64el-linux -- have I missed anything important here?
2012-08-01 Maciej W. Rozycki <macro@codesourcery.com>
ld/testsuite/
* ld-mips-elf/reloc-1-n32.d: Add -EB to GAS flags.
No need to update mips-elf.exp's entry as the update is intended to be
merged with the original change.
Maciej
binutils-mips-sde-test-newabi-el.diff
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-mips-elf/mips-elf.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/ld/testsuite/ld-mips-elf/mips-elf.exp 2012-08-01 17:20:56.000000000 +0100
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-mips-elf/mips-elf.exp 2012-08-01 17:41:53.621165721 +0100
@@ -357,14 +357,14 @@ if { $has_newabi } {
[list \
"reloc test 6a" \
"-shared $abi_ldflags(n32)" \
- "-n32" \
+ "$abi_asflags(n32)" \
"reloc-6a.s" \
{} \
"reloc-6a.so"] \
[list \
"reloc test 6b" \
"$abi_ldflags(n32) tmpdir/reloc-6a.so" \
- "-n32" \
+ "$abi_asflags(n32)" \
"reloc-6b.s" \
{} \
"reloc-6b"]]
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-mips-elf/reloc-1-n32.d
===================================================================
--- binutils-fsf-trunk-quilt.orig/ld/testsuite/ld-mips-elf/reloc-1-n32.d 2012-07-21 05:03:06.000000000 +0100
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-mips-elf/reloc-1-n32.d 2012-08-01 17:40:56.580512731 +0100
@@ -1,5 +1,5 @@
-#source: reloc-1a.s -mabi=n32
-#source: reloc-1b.s -mabi=n32
+#source: reloc-1a.s -mabi=n32 -EB
+#source: reloc-1b.s -mabi=n32 -EB
#ld: -r
#readelf: --relocs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MIPS: Enable NewABI tests for SDE targets
2012-08-01 14:43 [PATCH] MIPS: Enable NewABI tests for SDE targets Maciej W. Rozycki
2012-08-01 18:20 ` Maciej W. Rozycki
@ 2012-08-02 18:39 ` Richard Sandiford
2012-08-03 21:46 ` Maciej W. Rozycki
1 sibling, 1 reply; 6+ messages in thread
From: Richard Sandiford @ 2012-08-02 18:39 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Alan Modra, binutils
> Index: binutils-fsf-trunk-quilt/ld/testsuite/lib/ld-lib.exp
> ===================================================================
> --- binutils-fsf-trunk-quilt.orig/ld/testsuite/lib/ld-lib.exp 2012-07-24 15:29:41.000000000 +0100
> +++ binutils-fsf-trunk-quilt/ld/testsuite/lib/ld-lib.exp 2012-07-26 03:08:32.951798913 +0100
> @@ -431,7 +431,7 @@ proc ld_simple_link_defsyms {} {
> return $flags
> }
>
> -# run_dump_test FILE
> +# run_dump_test FILE (optional:) EXTRA_OPTIONS
> # Copied from gas testsuite, tweaked and further extended.
> #
> # Assemble a .s file, then run some utility on it and check the output.
> @@ -456,6 +456,12 @@ proc ld_simple_link_defsyms {} {
> # list ends with the first line that doesn't match the above syntax
> # (hmm, not great for error detection).
> #
> +# The optional EXTRA_OPTIONS argument to `run_dump_test' is a list of
> +# two-element lists. The first element of each is an option name, and
> +# the second additional arguments to be added on to the end of the
> +# option list as given in FILE.d. (If omitted, no additional options
> +# are added.)
> +#
> # The interesting options are:
> #
> # name: TEST-NAME
> @@ -503,6 +509,11 @@ proc ld_simple_link_defsyms {} {
> # More than one "source" directive can be given, which is useful
> # when testing linking.
> #
> +# dump: DUMP
> +# Match against DUMP.d. If omitted, this defaults to FILE.d. This
> +# is useful if several .d files differ by options only. Options are
> +# always read from FILE.d.
> +#
> # xfail: TARGET
> # The test is expected to fail on TARGET. This may occur more than
> # once.
> @@ -534,7 +545,7 @@ proc ld_simple_link_defsyms {} {
> # regexps in FILE.d. `regexp_diff' is defined in binutils-common.exp;
> # see further comments there.
> #
> -proc run_dump_test { name } {
> +proc run_dump_test { name {extra_options {}} } {
> global subdir srcdir
> global OBJDUMP NM AS OBJCOPY READELF LD
> global OBJDUMPFLAGS NMFLAGS ASFLAGS OBJCOPYFLAGS READELFFLAGS LDFLAGS
I think extra_options ought to be a one-level list, i.e.
{key1 value key2 value2 ...}. That makes the calls simpler
(because there's one fewer [list ...]) and:
> + foreach i $extra_options {
> + set opt_name [lindex $i 0]
> + set opt_val [lindex $i 1]
> + if ![info exists opts($opt_name)] {
> + perror "unknown option $opt_name given in extra_opts"
> + unresolved $subdir/$name
> + return
> + }
> + # Add extra option to end of existing option, adding space
> + # if necessary.
> + if { ![regexp "warning|error" $opt_name]
> + && [string length $opts($opt_name)] } {
> + append opts($opt_name) " "
> + }
> + append opts($opt_name) $opt_val
> + }
reduces to:
foreach { opt_name opt_val } $extra_options {
if ![info exists opts($opt_name)] {
perror "unknown option $opt_name given in extra_opts"
unresolved $subdir/$name
return
}
# Add extra option to end of existing option, adding space
# if necessary.
if { ![regexp "warning|error" $opt_name]
&& [string length $opts($opt_name)] } {
append opts($opt_name) " "
}
append opts($opt_name) $opt_val
}
OK with that change, thanks. OK for the follow-up too.
Richard
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MIPS: Enable NewABI tests for SDE targets
2012-08-02 18:39 ` Richard Sandiford
@ 2012-08-03 21:46 ` Maciej W. Rozycki
2012-08-04 12:07 ` Richard Sandiford
0 siblings, 1 reply; 6+ messages in thread
From: Maciej W. Rozycki @ 2012-08-03 21:46 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Alan Modra, binutils
On Thu, 2 Aug 2012, Richard Sandiford wrote:
> > Index: binutils-fsf-trunk-quilt/ld/testsuite/lib/ld-lib.exp
> > ===================================================================
> > --- binutils-fsf-trunk-quilt.orig/ld/testsuite/lib/ld-lib.exp 2012-07-24 15:29:41.000000000 +0100
> > +++ binutils-fsf-trunk-quilt/ld/testsuite/lib/ld-lib.exp 2012-07-26 03:08:32.951798913 +0100
> > @@ -431,7 +431,7 @@ proc ld_simple_link_defsyms {} {
> > return $flags
> > }
> >
> > -# run_dump_test FILE
> > +# run_dump_test FILE (optional:) EXTRA_OPTIONS
> > # Copied from gas testsuite, tweaked and further extended.
> > #
> > # Assemble a .s file, then run some utility on it and check the output.
> > @@ -456,6 +456,12 @@ proc ld_simple_link_defsyms {} {
> > # list ends with the first line that doesn't match the above syntax
> > # (hmm, not great for error detection).
> > #
> > +# The optional EXTRA_OPTIONS argument to `run_dump_test' is a list of
> > +# two-element lists. The first element of each is an option name, and
> > +# the second additional arguments to be added on to the end of the
> > +# option list as given in FILE.d. (If omitted, no additional options
> > +# are added.)
> > +#
> > # The interesting options are:
> > #
> > # name: TEST-NAME
> > @@ -503,6 +509,11 @@ proc ld_simple_link_defsyms {} {
> > # More than one "source" directive can be given, which is useful
> > # when testing linking.
> > #
> > +# dump: DUMP
> > +# Match against DUMP.d. If omitted, this defaults to FILE.d. This
> > +# is useful if several .d files differ by options only. Options are
> > +# always read from FILE.d.
> > +#
> > # xfail: TARGET
> > # The test is expected to fail on TARGET. This may occur more than
> > # once.
> > @@ -534,7 +545,7 @@ proc ld_simple_link_defsyms {} {
> > # regexps in FILE.d. `regexp_diff' is defined in binutils-common.exp;
> > # see further comments there.
> > #
> > -proc run_dump_test { name } {
> > +proc run_dump_test { name {extra_options {}} } {
> > global subdir srcdir
> > global OBJDUMP NM AS OBJCOPY READELF LD
> > global OBJDUMPFLAGS NMFLAGS ASFLAGS OBJCOPYFLAGS READELFFLAGS LDFLAGS
>
> I think extra_options ought to be a one-level list, i.e.
> {key1 value key2 value2 ...}. That makes the calls simpler
> (because there's one fewer [list ...]) and:
>
> > + foreach i $extra_options {
> > + set opt_name [lindex $i 0]
> > + set opt_val [lindex $i 1]
> > + if ![info exists opts($opt_name)] {
> > + perror "unknown option $opt_name given in extra_opts"
> > + unresolved $subdir/$name
> > + return
> > + }
> > + # Add extra option to end of existing option, adding space
> > + # if necessary.
> > + if { ![regexp "warning|error" $opt_name]
> > + && [string length $opts($opt_name)] } {
> > + append opts($opt_name) " "
> > + }
> > + append opts($opt_name) $opt_val
> > + }
>
> reduces to:
>
> foreach { opt_name opt_val } $extra_options {
> if ![info exists opts($opt_name)] {
> perror "unknown option $opt_name given in extra_opts"
> unresolved $subdir/$name
> return
> }
> # Add extra option to end of existing option, adding space
> # if necessary.
> if { ![regexp "warning|error" $opt_name]
> && [string length $opts($opt_name)] } {
> append opts($opt_name) " "
> }
> append opts($opt_name) $opt_val
> }
>
> OK with that change, thanks. OK for the follow-up too.
A list of lists seems more in the spirit of TCL to me, while a vector is
more C-like, but I have no strong preference either way. However please
note that implementing your suggestion will make the two run_dump_test
implementations confusingly different -- do you still want me to proceed?
If so, wouldn't it make sense to update the GAS version and any callers
accordingly so that the two versions don't diverge?
I wonder if the two couldn't simply be built around a common core moved
over to binutils/testsuite/lib/binutils-common.exp instead. There's not
much difference really and TCL has ways do such stuff more easily than for
example C.
Also some calls to "subst" over key values, specifically "as" and "ld"
(set opt_val [subst $opt_val]), look mysterious to me. While they might
be useful for options to have parts referring to TCL variables, this
however is not going to be particularly useful given that they are
executed in the context of run_dump_test. It looks to me it would make
more sense if they were executed in the caller's context with "uplevel".
Do you or anybody know what the original purpose of this substitution was?
Maciej
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MIPS: Enable NewABI tests for SDE targets
2012-08-03 21:46 ` Maciej W. Rozycki
@ 2012-08-04 12:07 ` Richard Sandiford
2012-08-06 21:39 ` Maciej W. Rozycki
0 siblings, 1 reply; 6+ messages in thread
From: Richard Sandiford @ 2012-08-04 12:07 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Alan Modra, binutils
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> On Thu, 2 Aug 2012, Richard Sandiford wrote:
>> > Index: binutils-fsf-trunk-quilt/ld/testsuite/lib/ld-lib.exp
>> > ===================================================================
>> > --- binutils-fsf-trunk-quilt.orig/ld/testsuite/lib/ld-lib.exp 2012-07-24 15:29:41.000000000 +0100
>> > +++ binutils-fsf-trunk-quilt/ld/testsuite/lib/ld-lib.exp 2012-07-26 03:08:32.951798913 +0100
>> > @@ -431,7 +431,7 @@ proc ld_simple_link_defsyms {} {
>> > return $flags
>> > }
>> >
>> > -# run_dump_test FILE
>> > +# run_dump_test FILE (optional:) EXTRA_OPTIONS
>> > # Copied from gas testsuite, tweaked and further extended.
>> > #
>> > # Assemble a .s file, then run some utility on it and check the output.
>> > @@ -456,6 +456,12 @@ proc ld_simple_link_defsyms {} {
>> > # list ends with the first line that doesn't match the above syntax
>> > # (hmm, not great for error detection).
>> > #
>> > +# The optional EXTRA_OPTIONS argument to `run_dump_test' is a list of
>> > +# two-element lists. The first element of each is an option name, and
>> > +# the second additional arguments to be added on to the end of the
>> > +# option list as given in FILE.d. (If omitted, no additional options
>> > +# are added.)
>> > +#
>> > # The interesting options are:
>> > #
>> > # name: TEST-NAME
>> > @@ -503,6 +509,11 @@ proc ld_simple_link_defsyms {} {
>> > # More than one "source" directive can be given, which is useful
>> > # when testing linking.
>> > #
>> > +# dump: DUMP
>> > +# Match against DUMP.d. If omitted, this defaults to FILE.d. This
>> > +# is useful if several .d files differ by options only. Options are
>> > +# always read from FILE.d.
>> > +#
>> > # xfail: TARGET
>> > # The test is expected to fail on TARGET. This may occur more than
>> > # once.
>> > @@ -534,7 +545,7 @@ proc ld_simple_link_defsyms {} {
>> > # regexps in FILE.d. `regexp_diff' is defined in binutils-common.exp;
>> > # see further comments there.
>> > #
>> > -proc run_dump_test { name } {
>> > +proc run_dump_test { name {extra_options {}} } {
>> > global subdir srcdir
>> > global OBJDUMP NM AS OBJCOPY READELF LD
>> > global OBJDUMPFLAGS NMFLAGS ASFLAGS OBJCOPYFLAGS READELFFLAGS LDFLAGS
>>
>> I think extra_options ought to be a one-level list, i.e.
>> {key1 value key2 value2 ...}. That makes the calls simpler
>> (because there's one fewer [list ...]) and:
>>
>> > + foreach i $extra_options {
>> > + set opt_name [lindex $i 0]
>> > + set opt_val [lindex $i 1]
>> > + if ![info exists opts($opt_name)] {
>> > + perror "unknown option $opt_name given in extra_opts"
>> > + unresolved $subdir/$name
>> > + return
>> > + }
>> > + # Add extra option to end of existing option, adding space
>> > + # if necessary.
>> > + if { ![regexp "warning|error" $opt_name]
>> > + && [string length $opts($opt_name)] } {
>> > + append opts($opt_name) " "
>> > + }
>> > + append opts($opt_name) $opt_val
>> > + }
>>
>> reduces to:
>>
>> foreach { opt_name opt_val } $extra_options {
>> if ![info exists opts($opt_name)] {
>> perror "unknown option $opt_name given in extra_opts"
>> unresolved $subdir/$name
>> return
>> }
>> # Add extra option to end of existing option, adding space
>> # if necessary.
>> if { ![regexp "warning|error" $opt_name]
>> && [string length $opts($opt_name)] } {
>> append opts($opt_name) " "
>> }
>> append opts($opt_name) $opt_val
>> }
>>
>> OK with that change, thanks. OK for the follow-up too.
>
> A list of lists seems more in the spirit of TCL to me, while a vector is
> more C-like
Not sure about that. "foreach { a b .. } ..." is provided exactly to make
flat lists easy to handle. [string map ...] takes a flat list rather than
a list of lists. Flat lists are also what is used by [array set ...]
and [array get ...], so that one easy way of handling unordered key-value
list arguments is:
array set foo $argument
if { [info exists $foo(key)] } ...
If you're lucky enough to be able to rely on Tcl 8.5, then dict operates
in just the same way. Flat lists can even be used as dicts directly,
without any conversion.
I don't know of any native Tcl operation that uses lists of lists instead.
> However please note that implementing your suggestion will make the
> two run_dump_test implementations confusingly different -- do you
> still want me to proceed?
That's a clincher though. Please go with your original patch.
Richard
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MIPS: Enable NewABI tests for SDE targets
2012-08-04 12:07 ` Richard Sandiford
@ 2012-08-06 21:39 ` Maciej W. Rozycki
0 siblings, 0 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2012-08-06 21:39 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Alan Modra, binutils
On Sat, 4 Aug 2012, Richard Sandiford wrote:
> > A list of lists seems more in the spirit of TCL to me, while a vector is
> > more C-like
>
> Not sure about that. "foreach { a b .. } ..." is provided exactly to make
> flat lists easy to handle. [string map ...] takes a flat list rather than
> a list of lists. Flat lists are also what is used by [array set ...]
> and [array get ...], so that one easy way of handling unordered key-value
> list arguments is:
>
> array set foo $argument
> if { [info exists $foo(key)] } ...
>
> If you're lucky enough to be able to rely on Tcl 8.5, then dict operates
> in just the same way. Flat lists can even be used as dicts directly,
> without any conversion.
>
> I don't know of any native Tcl operation that uses lists of lists instead.
They're easily traversed and many examples in the reference manual use
nested lists, but your arguments sound convincing to me, I agree.
> > However please note that implementing your suggestion will make the
> > two run_dump_test implementations confusingly different -- do you
> > still want me to proceed?
>
> That's a clincher though. Please go with your original patch.
I have therefore applied it now (and the update, although separately,
sorry about that), thanks for your review. Let's wait for some rainy
evening to get the two procedures converted to a flat list.
Maciej
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-08-06 21:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-01 14:43 [PATCH] MIPS: Enable NewABI tests for SDE targets Maciej W. Rozycki
2012-08-01 18:20 ` Maciej W. Rozycki
2012-08-02 18:39 ` Richard Sandiford
2012-08-03 21:46 ` Maciej W. Rozycki
2012-08-04 12:07 ` Richard Sandiford
2012-08-06 21:39 ` Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).