Re: [PATCH 0/5] target/i386: fxtract, fscale fixes

[Joseph Myers]

On Thu, 7 May 2020, address@hidden wrote:

> === OUTPUT BEGIN ===
> 1/5 Checking commit 69eed0bcaaaf (target/i386: implement special cases for 
> fxtract)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?

I don't think any MAINTAINERS update is needed for a new testcase in an 
existing directory.

> ERROR: Use of volatile is usually wrong, please add a comment

I think the justification for volatile in such testcase code is obvious 
without comments in individual cases - to avoid any code movement or 
optimization that might break what the tests are intending to test (these 
tests are making heavy use of mixed C and inline asm to test how emulated 
instructions behave, including on input representations that are not valid 
long double values in the ABI and with the rounding precision changed 
behind the compiler's back).  I think making everything possibly relevant 
volatile in these tests is better than trying to produce a fragile 
argument that in fact certain data does not need to be volatile to avoid 
problematic code movement.

> ERROR: spaces required around that '-' (ctx:VxV)
> #139: FILE: tests/tcg/i386/test-i386-fxtract.c:80:
> +                      "0" (0x1p-16445L));
>                                 ^

No, this is a C99 hex float contstant, not a subtraction.  There are 
already such constants in tests/tcg/multiarch/float_helpers.c and 
tests/tcg/multiarch/float_madds.c at least, so I assume they are OK in 
QEMU floating-point tests and this style checker should not be objecting 
to them.

-- 
Joseph S. Myers
address@hidden