|
From: | Milica Lazarevic |
Subject: | Re: [PATCH v2 12/20] disas/nanomips: Replace std::string type |
Date: | Thu, 8 Sep 2022 21:46:45 +0000 |
Thanks, it's more clear to me now! I'll modify it in the next by the suggestions.
From: Richard Henderson <richard.henderson@linaro.org>
Sent: Thursday, September 8, 2022 11:14 PM To: Milica Lazarevic <Milica.Lazarevic@Syrmia.com>; thuth@redhat.com <thuth@redhat.com> Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; cfontana@suse.de <cfontana@suse.de>; berrange@redhat.com <berrange@redhat.com>; pbonzini@redhat.com <pbonzini@redhat.com>; vince.delvecchio@mediatek.com <vince.delvecchio@mediatek.com>; peter.maydell@linaro.org <peter.maydell@linaro.org>; Djordje Todorovic <Djordje.Todorovic@syrmia.com>; mips32r2@gmail.com <mips32r2@gmail.com>; Dragan Mladjenovic <Dragan.Mladjenovic@syrmia.com> 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~ |
[Prev in Thread] | Current Thread | [Next in Thread] |