This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [RFC] Use only dwarf_vma types in dwarf code (was RE: [RFC patch]: Adjust the use of 'long' type in dwarf2.h header)


> > + ? ?if (byte_size > 0 && byte_size <=8)
> 
> Whitespace issue. should be ... && byte_size <= 8)

  Indeed, I still make silly formatting errors, sorry.
 
> > + ? ? ?offset = 16 - 2 * byte_size;
> > + ? ?else
> > + ? ? ?error("Wrong size in print_dwarf_vma");
> > +
> > + ?fputs (buff + offset, stdout);
> > ?}
> >
> > +#if __STDC_VERSION__ >= 199901L || (defined(__GNUC__) && __GNUC__ >=
> 2)
> > +#ifndef __MSVCRT__
> Please don't use here __MSVCRT__, instead use
> #if !defined (__MSVCRT__) && !defined (__MINGW32__)
> 
> As __MSVCRT__ is just declared by VC, and __MINGW32__ for gcc. IMHO
> __MINGW32__ should be the only guard necessary here, as VC neither
> defines __STDC_VERSION__ > 199901L and it doesn't defines __GNUC__,
> too.

  This is just a copy of what is already present 
in dwarf_print_vma function a few lines above in the patched 
file, so that we should fixed both occurrences 
 
> > +#define ?DWARF_VMA_FMT "ll"
> > +#else
> > +#define ?DWARF_VMA_FMT "I64"
> > +#endif
> > +#else
> > +#define ?DWARF_VMA_FMT "l"
> > +#endif
> > +
> 
> > -bfd_vma
> > +static char *
> > +dwarf_svmatoa (const char *fmtch, dwarf_signed_vma value)
> 
> Why you are using here dwarf_svmatoa? It should be dwarf_vmatoa
> As the formatter is specified here as argument and in dwarf_vmatoa the
> formatter-buffer is constructed, I see no point in adding a signed
> variant.

  I was worried about possible signed values in a signed type
that is shorter than dwarf_vma...
  Suppose that you have a 32-bit integer that has a value of -5,
but this is probably due to my lack of knowledge of C integer
signed<->unsigned conversion rules...
  I made a short check and it seems that indeed this is not necessary...

 
> This is just a first glance, and well, I can't approve this patch. But
> in general I appreachiate your modification.

  Thanks for your appreciation!
I will send a modified version of the patch shortly.

Pierre


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]