Thanks, it's more clear to me now! I'll modify it in the next by the
suggestions.
________________________________
From: Richard Henderson <[email protected]>
Sent: Thursday, September 8, 2022 11:14 PM
To: Milica Lazarevic <[email protected]>; [email protected]
<[email protected]>
Cc: [email protected] <[email protected]>; [email protected]
<[email protected]>; [email protected] <[email protected]>;
[email protected] <[email protected]>; [email protected]
<[email protected]>; [email protected]
<[email protected]>; Djordje Todorovic <[email protected]>;
[email protected] <[email protected]>; Dragan Mladjenovic
<[email protected]>
Subject: Re: [PATCH v2 12/20] disas/nanomips: Replace std::string type
On 9/8/22 20:16, Milica Lazarevic wrote:
> This would be better written as
>
> char *reg_list[33];
>
> assert(count <= 32);
> for (c = 0; c < count; c++) {
> bool use_gp = ...
> uint64 this_rt = ...
> /* glib usage below requires casting away const */
> reg_list[c] = (char *)GPR(this_rt);
> }
> reg_list[count] = NULL;
>
> return g_strjoinv(",", reg_list);
>
>
> In the implementation you suggested, there's one comma missing in the output.
> For example, instead of having:
> > 0x802021ce: 1e12 SAVE 0x10,ra,s0
> We're having this:
> < 0x802021ce: 1e12 SAVE 0x10ra,s0
Oh, right, because SAVE of zero registers is legal, and even useful as an
adjustment to
the stack pointer.
> So, I'm assuming that there needs to exist one more concatenation between the
> comma and
> the result of the g_strjoinv function?
> Maybe something like
> return g_strconcat((char *)",", (char *)g_strjoinv(",", reg_list), NULL);
Well, written like that you'd leak the result of g_strjoinv.
A better solution is to first element of reg_list be "", so that it's still
just a single
memory allocation.
> I think this interface should be
>
> char **dis,
>
> so that...
>
> > @@ -746,25 +647,26 @@ static int Disassemble(const uint16 *data,
> std::string & dis,
> > * an ASE attribute and the requested
> version
> > * not having that attribute
> > */
> > - dis = "ASE attribute mismatch";
> > + strcpy(dis, "ASE attribute mismatch");
>
> these become
>
> *dis = g_strdup("string");
>
> and the usage in nanomips_dis does not assume a fixed sized buffer.
>
>
> r~
>
>
> I'm not sure why the fixed size buffer would be a problem here since the
> buffer size has
> already been limited by the caller.
> I.e. in the print_insn_nanomips function, the buf variable is defined as:
> char buf[200];
There would be no such declaration with the above change.
r~
