[Qemu-devel] [PATCH] disas/arm: Remove redefinition of ATTRIBUTE_UNUSED

[Qemu-devel] [PATCH] disas/arm: Remove redefinition of ATTRIBUTE_UNUSED

[Tobias Klauser]

ATTRIBUTE_UNUSED is already defined in disas/bfd.h, which is included.
Thus, there is no need to redefine it.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 disas/arm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/disas/arm.c b/disas/arm.c
index 76e97a8..6f63406 100644
--- a/disas/arm.c
+++ b/disas/arm.c
@@ -23,7 +23,6 @@
    for things we don't care about.  */
 
 #include "disas/bfd.h"
-#define ATTRIBUTE_UNUSED __attribute__((unused))
 #define ISSPACE(x) ((x) == ' ' || (x) == '\t' || (x) == '\n')
 
 #define ARM_EXT_V1	 0
-- 
2.0.1

Re: [Qemu-devel] [Qemu-trivial] [PATCH] disas/arm: Remove redefinition of ATTRIBUTE_UNUSED

[Michael Tokarev]

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.

Oh well, so much for so trivial thing... ;)

/mjt

> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> ---
>  disas/arm.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/disas/arm.c b/disas/arm.c
> index 76e97a8..6f63406 100644
> --- a/disas/arm.c
> +++ b/disas/arm.c
> @@ -23,7 +23,6 @@
>     for things we don't care about.  */
>  
>  #include "disas/bfd.h"
> -#define ATTRIBUTE_UNUSED __attribute__((unused))
>  #define ISSPACE(x) ((x) == ' ' || (x) == '\t' || (x) == '\n')
>  
>  #define ARM_EXT_V1	 0
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH] disas/arm: Remove redefinition of ATTRIBUTE_UNUSED

[Peter Maydell]

On 24 September 2014 00:58, Michael Tokarev <mjt@tls.msk.ru> 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...

-- PMM

Re: [Qemu-devel] [Qemu-trivial] [PATCH] disas/arm: Remove redefinition of ATTRIBUTE_UNUSED

[Tobias Klauser]

On 2014-09-24 at 09:58:45 +0200, Michael Tokarev <mjt@tls.msk.ru> 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?

No.

> 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.

Ah, I didn't know it was non-qemu code (but now that I look at it again,
I probably should have realized that sooner ;). Thanks for the
explanation.

> Oh well, so much for so trivial thing... ;)

:) Sorry, it wasn't my intention to cause much discussion, trouble or
work for maintainers with that patch. I don't feel strongly about, I
just stumbled across it when browsing the source and found it odd to see
a macro defined just below the #include of a header which defines the
same macro.

Please just ignore the patch then :)

Thanks
Tobias

Re: [Qemu-devel] [Qemu-trivial] [PATCH] disas/arm: Remove redefinition of ATTRIBUTE_UNUSED

[Tobias Klauser]

On 2014-09-24 at 11:09:42 +0200, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 September 2014 00:58, Michael Tokarev <mjt@tls.msk.ru> 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