Re: [PATCH v2 32/65] MMIX: use is_whitespace()

[Jan Beulich <jbeulich@suse.com>]

On 05.02.2025 07:22, Hans-Peter Nilsson wrote:
> On Mon, 3 Feb 2025, Jan Beulich wrote:
>> On 03.02.2025 07:54, Hans-Peter Nilsson wrote:
>>> On Mon, 27 Jan 2025, Jan Beulich wrote:
>>>> Convert open-coded checks as well as ISSPACE() uses. At the same time
>>>> use is_end_of_stmt() instead of open-coded checks; do the conversion
>>>> even when not adjacent to code being modified anyway to cover all cases
>>>> where the is_end_of_line[] index was wrongly cast from plain char (which
>>>> can be signed) to unsigned int.
>>>> ---
>>>> v2: New.
>>>>
>>>> --- a/gas/config/tc-mmix.c
>>>> +++ b/gas/config/tc-mmix.c
>>>
>>> Sorry, there are a few hunks that are wrong, like missing a test 
>>> for ';' and other changes that may work but doesn't logically 
>>> make sense to me.
>>
>> Missing ';' checks? The patch replaces a few with is_end_of_stmt(), yes,
>> but that's intentional (to stop its open-coding).
> 
> Oh right, I'd looked at the end_of_line[] definition and forgot 
> about MMIX adding ';' by means of line_separator_chars[]. In the 
> end, all code-changes were ok though with the caveat of now 
> supporting just tab and space.
> 
> Though bundling all that into saying "stop open-coding" was too 
> vague.  You were simply doing too much and still too little in 
> the same patch, making review harder than necessary.

Such mixing is pretty common in binutils changes. Other projects
I work on are more strict in this regard; I'm trying to find a
balance, but I don#t always succeed.

> Too much: "replacing open-coding" and removing redundant 
> checking while changing those whitespace tests; the IS... calls 
> to is_... and fixing erroneous casts.
> 
> Unfortunately there isn't a 1:1 equivalences, so the input 
> language is now different.  Now with is_whitespace it accepts 
> only ' ' and '\t' where previously ISSPACE covered more 
> whitespace characters in some places.  Not sure there are any 
> assembly language programmers using e.g. \f instead of \t, but 
> some of their code now no longer assembles for some targets.  
> Example: replace the space after the first 1H in 1hjmp1b.s with 
> a form-feed and run the related tests.  But, pragmatically, I'll 
> ok that change.  Nobody in their right mind should use other 
> than \t and ' ' as assembly-code field separators.  They had it 
> coming. 1/2 :-)

Well, I'm aware - see the two respective remarks in the cover
letter. ISSPACE() was problematic anyway, for including \r and
\n as well. ISBLANK() may have been slightly better. In any
event - we now have control over what we want to consider white
space. We can add \f and/or \v, or any others. My goal is though
that for no character is_whitespace() and is_end_of_stmt() both
yield true.

Jan