[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-trivial] [PATCH] disas/arm: Remove redefinition o
From: |
Tobias Klauser |
Subject: |
Re: [Qemu-devel] [Qemu-trivial] [PATCH] disas/arm: Remove redefinition of ATTRIBUTE_UNUSED |
Date: |
Wed, 24 Sep 2014 13:39:56 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On 2014-09-24 at 11:09:42 +0200, Peter Maydell <address@hidden> wrote:
> On 24 September 2014 00:58, Michael Tokarev <address@hidden> wrote:
> > 18.09.2014 21:25, Tobias Klauser wrote:
> >> ATTRIBUTE_UNUSED is already defined in disas/bfd.h, which is included.
> >> Thus, there is no need to redefine it.
> >
> > Is there any harm in keeping it here?
> >
> > While it really is a redifinition, I'm not sure what's the right thing
> > here. This whole code is not from qemu, it is an external source imported,
> > and that source is being maintained (but under different license as has
> > already been discussed, so keeping changes at minimum might not be that
> > good idea anymore). On the other hand this symbol is so common it should
> > be defined in a common header. Yet on another hand, for these external
> > sources wich has public API, it might not be a good idea to define it in
> > a header to start with, because it might clash with project-local define,
> > so it might be better to define it in either private header or in individual
> > source.
>
> So my take on the disas/ sources is:
> * yes, they're from an external source
> * but as you say, we're never going to take another drop from that
> external source so we should feel free to make local bugfixes
> and changes as we need to
> * on the other hand, there's no point in making gratuitous changes
> to them (they're never going to match the QEMU coding style,
> for instance)
>
> So what's the rationale for this particular change? The duplication
> is harmless, so why worry about it...
There's no particular reason, I just found it odd to see a redefinition
of the macro there and I didn't know about the external source of the
code - please just ignore the patch then.
Thanks
Tobias