[PATCH v2 15/17] MIPS: relax gas testsuite whitespace expectations

[Jan Beulich]

On 15.07.2024 14:12, Maciej W. Rozycki wrote:
> On Fri, 12 Jul 2024, Jan Beulich wrote:
> 
>> In a subsequent change the scrubber is going to be changed to retain
>> further whitespace. Test case expectations generally would better not
>> depend on the specific whitespace treatment by the scrubber, unless of
>> course a test is specifically about it. Adjust relevant test cases to
>> permit blanks where those will subsequently appear.
> 
>  I disagree.  I understand you might be unhappy to have to tediously go 
> and mechanically update a bunch of test cases because the spacing has 
> changed (well, actually you don't have: you can just produce replacement 
> dumps and just verify that the diff to the previous one is sane, which is 
> what I do on such occasions).  But this is occasional effort, not expected 
> to happen often, however the extra regexp characters make test patterns 
> harder for a human to read.
> 
>  Therefore I would rather the updated patterns reflected new formatting 
> verbatim, at least in the MIPS target I care about, and if we have a need 
> to change formatting again in 10 years' time, then so be it.

Which then means these changes here need to be part of the patch actually
changing the scrubber. I can do that, but I'd like to point out the
scalability issue with this: If all arch maintainers asked me to do so,
I'd end up with a gigantic patch.

>> ---
>> This is adding just the blanks that are going to be needed; imo it would
>> generally be better if test case expectations were, from the very
>> beginning, written to focus on what is being tested, rather than taking
>> verbatim copies of the respective tool's output.
> 
>  I'm sorry, how could the test case writer predict that extra whitespace 
> might be added in the future?

This is the wrong question to ask, imo. There's no whitespace being
"added". The issue is with listing tests which take verbatim the listing
output, when really they should further consider the original input to
gas. In principle any whitespace there may or may not survive scrubbing.

>  This statement makes no sense to me, this 
> way all tests should be rewritten such as to possibly expect whitespace 
> between tokens where there currently isn't any.

Listing tests should indeed have been written like this, imo.

>  Also I think if you retain whitespace, you should be doing this 
> consistently, i.e. if the source instruction is say:
> 
> 	dext	$2, $3, 1 << 2, 1 << 4
> 
> then in error reporting this should be used:
> 
> dext $2, $3, 1 << 2, 1 << 4
> 
> rather than this:
> 
> dext $2,$3,1 << 2,1 << 4
> 
> which makes the instruction reported hard to read/parse to a human, as you 
> suddenly have whitespace within operands, but not between them.  I guess 
> this is something to handle in 17/17.

No. I'm very deliberately special casing commas there, or else I'd have
to make yet more adjustments to the testsuite. It's already bad enough.
Plus who knows how many more target specific parsing adjustments I'd need
to make, to deal with the further "surviving" whitespace.

(As an aside, I'm also having trouble seeing why the whole insn, with all
operands, would need quoting in a diagnostic. There may be rare cases
whether this is necessary for context, but usually it should be sufficient
to mention just the part that's actually wrong.)

>  NAK for this part then.

I'm sorry, but no, I'm not going to drop all this work just because of you
not liking a tiny aspect. Best I can do then is to leave out this patch,
and leave fixing the MIPS testsuite to you. I'm not sure you're going to
be happy about this. Please let me know.

Jan