Re: [PATCH v3 16/24] disas/nanomips: Remove IMMEDIATE functions

[Philippe Mathieu-Daudé]

On 1/11/22 09:28, Stefan Weil via wrote:
> Am 12.09.22 um 14:26 schrieb Milica Lazarevic:
> > Both versions of IMMEDIATE functions have been removed.
> >
> > Before this patch, we'd been calling img_format twice, the first time
> > through the IMMEDIATE to get an appropriate string and the second time
> > to print that string. There's no more need for that. Therefore, calls to
> > IMMEDIATE are removed, and now we're directly printing the integer
> > values instead.
> >
> > Signed-off-by: Milica Lazarevic <milica.lazarevic@syrmia.com>
> > ---
> >   disas/nanomips.cpp | 756 ++++++++++++++++-----------------------------
> >   1 file changed, 265 insertions(+), 491 deletions(-)
> >
> > diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
> > index 816155527d..441204bb84 100644
> > --- a/disas/nanomips.cpp
> > +++ b/disas/nanomips.cpp
> [...]
> > @@ -3305,11 +3271,9 @@ static char *CACHE(uint64 instruction, Dis_info 
> > *info)
> >       uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
> >       int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
> > -    char *op = IMMEDIATE(op_value);
> > -    char *s = IMMEDIATE(s_value);
> >       const char *rs = GPR(rs_value);
> > -    return img_format("CACHE %s, %s(%s)", op, s, rs);
> > +    return img_format("CACHE 0x%" PRIx64 ", %s(%s)", op_value, 
> > s_value, rs);
> >   }
> > @@ -3329,11 +3293,9 @@ static char *CACHEE(uint64 instruction, 
> > Dis_info *info)
> >       uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
> >       int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
> > -    char *op = IMMEDIATE(op_value);
> > -    char *s = IMMEDIATE(s_value);
> >       const char *rs = GPR(rs_value);
> > -    return img_format("CACHEE %s, %s(%s)", op, s, rs);
> > +    return img_format("CACHEE 0x%" PRIx64 ", %s(%s)", op_value, 
> > s_value, rs);
> >   }
>
> Do we really want to format "int64 s_value" as a string? The code now 
> has lots of wrong format strings. Add the patch below to get the 
> compiler report.
>
> We once had a discussion about using G_GNUC_PRINTF for local functions 
> or not. I think that this example clearly shows that it should be 
> mandatory.

Yes. The problem here is nobody wants to maintain this code, but we
inherited it. IIUC this series doesn't make it worst, it just remove
the C++ dependency on UNIX-based hosts.

> Regards,
> Stefan
>
> diff --git a/disas/nanomips.c b/disas/nanomips.c
> index 9647f1a8e3..c875818cb9 100644
> --- a/disas/nanomips.c
> +++ b/disas/nanomips.c
> @@ -95,7 +95,7 @@ typedef struct Pool {
>   #define IMGASSERTONCE(test)
>
>
> -static char *img_format(const char *format, ...)
> +static char * G_GNUC_PRINTF(1, 2) img_format(const char *format, ...)
>   {
>       char *buffer;
>       va_list args;