From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-sender-0.a4lg.com (mail-sender.a4lg.com [153.120.152.154]) by sourceware.org (Postfix) with ESMTPS id AC0DA38582B6 for ; Wed, 10 Aug 2022 13:54:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AC0DA38582B6 Received: from [127.0.0.1] (localhost [127.0.0.1]) by mail-sender-0.a4lg.com (Postfix) with ESMTPSA id 7E008300089; Wed, 10 Aug 2022 13:54:52 +0000 (UTC) Message-ID: <8c9a39c2-7ad0-1a69-9781-be0d5c3d60b6@irq.a4lg.com> Date: Wed, 10 Aug 2022 22:54:51 +0900 Mime-Version: 1.0 Subject: Re: [PATCH 4/4] RISC-V: Print comma and tabs as the `text' style Content-Language: en-US To: Andrew Burgess References: <4919138bb21768d0cdc3a6da81dbbbd62cc96855.1657719581.git.research_trasio@irq.a4lg.com> <13432197297737fb0f449ec6ef123cfdc0bff8a1.1659500861.git.research_trasio@irq.a4lg.com> <87y1vwz6o4.fsf@redhat.com> Cc: Binutils From: Tsukasa OI In-Reply-To: <87y1vwz6o4.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Aug 2022 13:54:57 -0000 On 2022/08/10 20:20, Andrew Burgess wrote: > Tsukasa OI via Binutils writes: > >> On the RISC-V disassembler, some separators have non-`text' style when >> printed with another word with another style. >> >> This commit splits those, making sure that those comma and tabs are printed >> with the `text' style. >> >> opcodes/ChangeLog: >> >> * riscv-dis.c (print_insn_args): Split and print the comma as text. >> (riscv_disassemble_insn): Split and print tabs as text. >> (riscv_disassemble_data): Likewise. > > I'm not a binutils maintainer, so can't approve this patch. However, > this looks good to me with one minor nit, see below... > >> --- >> opcodes/riscv-dis.c | 31 ++++++++++++++++++++----------- >> 1 file changed, 20 insertions(+), 11 deletions(-) >> >> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c >> index fe18c18642e..460f77676d2 100644 >> --- a/opcodes/riscv-dis.c >> +++ b/opcodes/riscv-dis.c >> @@ -373,9 +373,12 @@ print_insn_args (const char *oparg, insn_t l, bfd_vma pc, disassemble_info *info >> (int)EXTRACT_RVV_OFFSET (l)); >> break; >> case 'm': >> - if (! EXTRACT_OPERAND (VMASK, l)) >> - print (info->stream, dis_style_register, ",%s", >> - riscv_vecm_names_numeric[0]); >> + if (!EXTRACT_OPERAND (VMASK, l)) >> + { >> + print (info->stream, dis_style_text, ","); >> + print (info->stream, dis_style_register, "%s", >> + riscv_vecm_names_numeric[0]); >> + } >> break; >> } >> break; >> @@ -709,7 +712,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) >> case 4: >> case 8: >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".%dbyte\t", insnlen); >> + (info->stream, dis_style_assembler_directive, ".%dbyte", insnlen); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> (*info->fprintf_styled_func) (info->stream, dis_style_immediate, >> "0x%llx", (unsigned long long) word); >> break; >> @@ -717,7 +721,8 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info) >> { >> int i; >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".byte\t"); >> + (info->stream, dis_style_assembler_directive, ".byte"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> for (i = 0; i < insnlen; ++i) >> { >> if (i > 0) >> @@ -905,21 +910,24 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, >> case 1: >> info->bytes_per_line = 6; >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".byte\t"); >> - (*info->fprintf_styled_func) >> - (info->stream, dis_style_immediate, "0x%02x", (unsigned) data); >> + (info->stream, dis_style_assembler_directive, ".byte"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_immediate, >> + "0x%02x", (unsigned)data); > > You lost the space in '(unsigned) data'. > > Thanks, > Andrew Interesting. I formatted this line using Visual Studio Code (which uses clang-format) and I think this is a minor difference between clang-format's "GNU" indent and real GNU Indent's defaults. Changing clang-format's settings just like "SpaceAfterCStyleCast: true" will mimic the default GNU Indent's behavior (better). It's too minor (this source file has other (type)(value) without any spaces) and will not republish the patchset unless there's some other reasons. But thank you for letting me know that there's something clang-format misses and I will follow "SpaceAfterCStyleCast: true" in the future. Thanks, Tsukasa > >> break; >> case 2: >> info->bytes_per_line = 8; >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".short\t"); >> + (info->stream, dis_style_assembler_directive, ".short"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> (*info->fprintf_styled_func) >> (info->stream, dis_style_immediate, "0x%04x", (unsigned) data); >> break; >> case 4: >> info->bytes_per_line = 8; >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".word\t"); >> + (info->stream, dis_style_assembler_directive, ".word"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> (*info->fprintf_styled_func) >> (info->stream, dis_style_immediate, "0x%08lx", >> (unsigned long) data); >> @@ -927,7 +935,8 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED, >> case 8: >> info->bytes_per_line = 8; >> (*info->fprintf_styled_func) >> - (info->stream, dis_style_assembler_directive, ".dword\t"); >> + (info->stream, dis_style_assembler_directive, ".dword"); >> + (*info->fprintf_styled_func) (info->stream, dis_style_text, "\t"); >> (*info->fprintf_styled_func) >> (info->stream, dis_style_immediate, "0x%016llx", >> (unsigned long long) data); >> -- >> 2.34.1 >