Re: [PATCH 2/6] Refactor disassembly code

[Yao Qi <qiyaoltc@gmail.com>]

On 17-01-17 08:14:27, Luis Machado wrote:
> >
> > /* Like target_read_memory, but slightly different parameters.  */
> 
> Should we update these comments to a more useful version? "slightly
> different parameters" doesn't explain much, as it is obvious they
> are slightly different. Other uses below have the same problem.

How about this?

  /* Wrapper of target_read_code.  */

> 
> >-static int
> >-dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
> >-		     struct disassemble_info *info)
> >+
> >+int
> >+gdb_disassembler::dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr,
> >+				       unsigned int len,
> >+				       struct disassemble_info *info)
> > {
> >   return target_read_code (memaddr, myaddr, len);
> > }
> >
> > /* Like memory_error with slightly different parameters.  */

and how about

/* Wrapper of memory_error.  */

> >-static void
> >-dis_asm_memory_error (int err, bfd_vma memaddr,
> >-		      struct disassemble_info *info)
> >+
> >+

> >+int
> >+gdb_disassembler::print_insn (CORE_ADDR memaddr,
> >+			      int *branch_delay_insns)
> >+{
> >+  int length = gdbarch_print_insn (arch (), memaddr, &m_di);
> >+
> >+  if (branch_delay_insns != NULL)
> >+    {
> >+      if (m_di.insn_info_valid)
> >+	*branch_delay_insns = m_di.branch_delay_insns;
> >+      else
> >+	*branch_delay_insns = 0;
> >+    }
> >+  return length;
> 
> length doesn't seem to have a purpose other than being returned. So
> just return gdbarch_print_insn (arch (), memaddr, &m_di)?
> 

branch_delay_insns needs update after gdbarch_print_insn call.

> >+
> >+  /* Prints the instruction INSN into UIOUT and returns the length of
> >+     the printed instruction in bytes.  */
> >+  int pretty_print_insn (struct ui_out *uiout,
> >+			 const struct disasm_insn *insn, int flags);
> 
> Can this function return negative? If not, use unsigned?
> 

I don't think pretty_print_insn can return negative, and we can change
it to size_t.  However, this patch is a code refactor.  I want to avoid
change like this in this patch.  I can change 'int' to 'size_t' later.

> >+
> >+  /* Return the gdbarch of gdb_disassembler.  */
> >+  struct gdbarch *arch ()
> >+  { return m_gdbarch; }
> >+
> >+protected:
> >+  gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file,
> >+		    di_read_memory_ftype func);
> >+
> >+  struct ui_file *stream ()
> >+  { return (struct ui_file *) m_di.stream; }
> >+
> >+private:
> >+  struct gdbarch *m_gdbarch;
> >+  struct disassemble_info m_di;
> 
> Add comments explaining what m_gdbarch and m_di are used for and/or how?

I don't know what kind of comments I can add for m_gdbarch.  Something
like "gdbarch of gdb_disassembler" looks useless.  We've already had
method arch(with comments) which returns m_gdbarch.

I can think of the comments to m_di

  /* Pass it to opcodes disassembler and collect some outputs
     from opcodes disassembler.  */

  struct disassemble_info m_di;

What do you think?

-- 
Yao (齐尧)