public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Tsukasa OI <research_trasio@irq.a4lg.com>
To: Andrew Burgess <aburgess@redhat.com>
Cc: Binutils <binutils@sourceware.org>
Subject: Re: [PATCH 4/4] RISC-V: Print comma and tabs as the `text' style
Date: Wed, 10 Aug 2022 22:54:51 +0900	[thread overview]
Message-ID: <8c9a39c2-7ad0-1a69-9781-be0d5c3d60b6@irq.a4lg.com> (raw)
In-Reply-To: <87y1vwz6o4.fsf@redhat.com>

On 2022/08/10 20:20, Andrew Burgess wrote:
> Tsukasa OI via Binutils <binutils@sourceware.org> 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
> 

  reply	other threads:[~2022-08-10 13:54 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-13 13:40 [PATCH] RISC-V: fix printf types on riscv-dis.c Tsukasa OI
2022-08-03  4:27 ` [PATCH 0/4] RISC-V: Fix disassembler types and styles Tsukasa OI
2022-08-03  4:27   ` [PATCH 1/4] RISC-V: Fix immediates to have `immediate' style Tsukasa OI
2022-08-10 11:16     ` Andrew Burgess
2022-08-10 13:48       ` Tsukasa OI
2022-08-03  4:27   ` [PATCH 2/4] RISC-V: Fix printf argument types corresponding %x Tsukasa OI
2022-08-03  4:27   ` [PATCH 3/4] RISC-V: Optimize riscv_disassemble_data printf Tsukasa OI
2022-08-03  4:27   ` [PATCH 4/4] RISC-V: Print comma and tabs as the `text' style Tsukasa OI
2022-08-10 11:20     ` Andrew Burgess
2022-08-10 13:54       ` Tsukasa OI [this message]
2022-09-26 12:26   ` [PATCH v2 0/6] RISC-V: Fix disassembler types and styles Tsukasa OI
2022-09-26 12:26     ` [PATCH v2 1/6] RISC-V: Fix immediates to have "immediate" style Tsukasa OI
2022-09-26 12:26     ` [PATCH v2 2/6] RISC-V: Fix printf argument types corresponding %x Tsukasa OI
2022-09-26 12:26     ` [PATCH v2 3/6] RISC-V: Optimize riscv_disassemble_data printf Tsukasa OI
2022-09-26 12:26     ` [PATCH v2 4/6] RISC-V: Print comma and tabs as the "text" style Tsukasa OI
2022-09-26 12:26     ` [PATCH v2 5/6] RISC-V: Fix T-Head immediate types on printing Tsukasa OI
2022-10-03  9:57       ` Andrew Burgess
2022-10-03 11:06       ` Christoph Müllner
2022-09-26 12:26     ` [PATCH v2 6/6] RISC-V: Print XTheadMemPair literal as "immediate" Tsukasa OI
2022-10-03 11:06       ` Christoph Müllner
2022-10-03  9:59     ` [PATCH v2 0/6] RISC-V: Fix disassembler types and styles Andrew Burgess
2022-10-03 17:40       ` Palmer Dabbelt
2022-10-04  1:34         ` Tsukasa OI
2022-10-04  1:41         ` Nelson Chu
2022-10-04  8:46         ` Andrew Burgess
2022-10-05 22:37           ` Palmer Dabbelt
2022-10-05 21:52         ` Jeff Law
2022-10-03 10:43     ` [PATCH v3 " Tsukasa OI
2022-10-03 10:43       ` [PATCH v3 1/6] RISC-V: Fix immediates to have "immediate" style Tsukasa OI
2022-10-03 10:44       ` [PATCH v3 2/6] RISC-V: Fix printf argument types corresponding %x Tsukasa OI
2022-10-03 10:44       ` [PATCH v3 3/6] RISC-V: Optimize riscv_disassemble_data printf Tsukasa OI
2022-10-03 10:44       ` [PATCH v3 4/6] RISC-V: Print comma and tabs as the "text" style Tsukasa OI
2022-10-03 10:44       ` [PATCH v3 5/6] RISC-V: Fix T-Head immediate types on printing Tsukasa OI
2022-10-03 10:44       ` [PATCH v3 6/6] RISC-V: Print XTheadMemPair literal as "immediate" Tsukasa OI

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8c9a39c2-7ad0-1a69-9781-be0d5c3d60b6@irq.a4lg.com \
    --to=research_trasio@irq.a4lg.com \
    --cc=aburgess@redhat.com \
    --cc=binutils@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).