[PATCH] sim: avoid TRACE redefine warnings

[PATCH] sim: avoid TRACE redefine warnings

[Mike Frysinger]

The common code sets up an autoconf option --enable-sim-trace which adds
-DTRACE= to CPPFLAGS.  This causes warnings in the building of some files
that declare a local TRACE() helper macro.  So punt it from hw-ports.c
(since it isn't actually used) and rename it in hw-properties.c.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
2010-03-14  Mike Frysinger  <vapier@gentoo.org>

	* hw-ports.c (TRACE): Delete.
	* hw-properties.c (TRACE): Rename to _TRACE.

 sim/common/hw-ports.c      |    2 --
 sim/common/hw-properties.c |   10 +++++-----
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/sim/common/hw-ports.c b/sim/common/hw-ports.c
index 8d5c14e..8f88cb3 100644
--- a/sim/common/hw-ports.c
+++ b/sim/common/hw-ports.c
@@ -35,8 +35,6 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include <ctype.h>
 
-#define TRACE(x,y)
-
 
 struct hw_port_edge {
   int my_port;
diff --git a/sim/common/hw-properties.c b/sim/common/hw-properties.c
index 3166e42..b7d4009 100644
--- a/sim/common/hw-properties.c
+++ b/sim/common/hw-properties.c
@@ -33,7 +33,7 @@
 #endif
 #endif
 
-#define TRACE(A,B)
+#define _TRACE(A,B)
 
 /* property entries */
 
@@ -418,7 +418,7 @@ hw_find_ihandle_runtime_property (struct hw *me,
 				  ihandle_runtime_property_spec *ihandle)
 {
   struct hw_property_data *entry = find_property_data (me, property);
-  TRACE (trace_devices,
+  _TRACE (trace_devices,
 	 ("hw_find_ihandle_runtime_property(me=0x%lx, property=%s)\n",
 	  (long)me, property));
   if (entry == NULL)
@@ -492,7 +492,7 @@ hw_find_integer_property (struct hw *me,
 {
   const struct hw_property *node;
   signed_cell integer;
-  TRACE (trace_devices,
+  _TRACE (trace_devices,
 	 ("hw_find_integer(me=0x%lx, property=%s)\n",
 	  (long)me, property));
   node = hw_find_property (me, property);
@@ -514,7 +514,7 @@ hw_find_integer_array_property (struct hw *me,
   const struct hw_property *node;
   int sizeof_integer = sizeof (*integer);
   signed_cell *cell;
-  TRACE (trace_devices,
+  _TRACE (trace_devices,
 	 ("hw_find_integer(me=0x%lx, property=%s)\n",
 	  (long)me, property));
   
@@ -886,7 +886,7 @@ hw_add_duplicate_property (struct hw *me,
 			   const struct hw_property *original)
 {
   struct hw_property_data *master;
-  TRACE (trace_devices,
+  _TRACE (trace_devices,
 	 ("hw_add_duplicate_property(me=0x%lx, property=%s, ...)\n",
 	  (long)me, property));
   if (original->disposition != permenant_object)
-- 
1.7.0

Re: [PATCH] sim: avoid TRACE redefine warnings

[Joel Brobecker]

> 2010-03-14  Mike Frysinger  <vapier@gentoo.org>
> 
> 	* hw-ports.c (TRACE): Delete.
> 	* hw-properties.c (TRACE): Rename to _TRACE.

The change in hw-ports.c is OK, but I don't get the change in
hw-properties.c:

> -#define TRACE(A,B)
> +#define _TRACE(A,B)

Can you explain?

-- 
Joel

Re: [PATCH] sim: avoid TRACE redefine warnings

[Pedro Alves]

On Sunday 14 March 2010 20:39:16, Mike Frysinger wrote:
> -#define TRACE(A,B)
> +#define _TRACE(A,B)

Identifier names beggining with an underscore followed by an
uppercase letter are reserved for the implementation.  Let's avoid
them.

-- 
Pedro Alves

Re: [PATCH] sim: avoid TRACE redefine warnings

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 420 bytes --]

On Sunday 14 March 2010 20:43:38 Pedro Alves wrote:
> On Sunday 14 March 2010 20:39:16, Mike Frysinger wrote:
> > -#define TRACE(A,B)
> > +#define _TRACE(A,B)
> 
> Identifier names beggining with an underscore followed by an
> uppercase letter are reserved for the implementation.  Let's avoid
> them.

i dont know what "the implementation" means.  this looks like "an 
implementation" of a simulator.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sim: avoid TRACE redefine warnings

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 667 bytes --]

On Sunday 14 March 2010 20:35:38 Joel Brobecker wrote:
> > 2010-03-14  Mike Frysinger  <vapier@gentoo.org>
> > 
> > 	* hw-ports.c (TRACE): Delete.
> > 	* hw-properties.c (TRACE): Rename to _TRACE.
> 
> The change in hw-ports.c is OK, but I don't get the change in
> 
> hw-properties.c:
> > -#define TRACE(A,B)
> > +#define _TRACE(A,B)
> 
> Can you explain?

as i mentioned in the first e-mail, "TRACE" is defined on the command line, so 
any macro named "TRACE" has to be renamed.  since this file actually used the 
macro, i renamed them all.  i could simply delete all TRACE usage in the file 
if you prefer ... doesnt really matter to me.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sim: avoid TRACE redefine warnings

[Matt Rice]

On Mon, Mar 15, 2010 at 12:18 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Sunday 14 March 2010 20:43:38 Pedro Alves wrote:
>> On Sunday 14 March 2010 20:39:16, Mike Frysinger wrote:
>> > -#define TRACE(A,B)
>> > +#define _TRACE(A,B)
>>
>> Identifier names beggining with an underscore followed by an
>> uppercase letter are reserved for the implementation.  Let's avoid
>> them.
>
> i dont know what "the implementation" means.  this looks like "an
> implementation" of a simulator.
> -mike
>

http://www.gnu.org/s/libc/manual/html_node/Reserved-Names.html

Re: [PATCH] sim: avoid TRACE redefine warnings

[Pedro Alves]

On Monday 15 March 2010 07:18:28, Mike Frysinger wrote:
> i dont know what "the implementation" means.  this looks like "an 
> implementation" of a simulator.

Sorry, I meant, "the C implementation".

-- 
Pedro Alves

Re: [PATCH] sim: avoid TRACE redefine warnings

[Joel Brobecker]

> as i mentioned in the first e-mail, "TRACE" is defined on the command
> line, so any macro named "TRACE" has to be renamed.

I understood why you had to rename the macro.  The problem is that
I'm wondering what this macro is supposed to be used for.  It's important
because I suspect the change that you made might be incorrect, depending
on the answer to the question above. For instance, I suspect that the
use of the TRACE macro in that file might be related to
--enable-sim-traces, and thus renaming TRACE to something else will break
that.

That's why, IMO, we need to:
  - Understand how --enable-sim-traces work to see if it is related
    to the use of this TRACE macro;
  - Find out how to properly fix your problem;

If you want, I can try doing some digging, but I think it's going to be
a while before I can get to it, which is why I'm giving you as much info
as I have (I really do not have much sim experience, but apparently we
are short in reviewers for the sim code).

-- 
Joel

Re: [PATCH] sim: avoid TRACE redefine warnings

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 2266 bytes --]

On Monday 15 March 2010 12:49:34 Joel Brobecker wrote:
> > as i mentioned in the first e-mail, "TRACE" is defined on the command
> > line, so any macro named "TRACE" has to be renamed.
> 
> I understood why you had to rename the macro.  The problem is that
> I'm wondering what this macro is supposed to be used for.  It's important
> because I suspect the change that you made might be incorrect, depending
> on the answer to the question above. For instance, I suspect that the
> use of the TRACE macro in that file might be related to
> --enable-sim-traces, and thus renaming TRACE to something else will break
> that.
> 
> That's why, IMO, we need to:
>   - Understand how --enable-sim-traces work to see if it is related
>     to the use of this TRACE macro;
>   - Find out how to properly fix your problem;
> 
> If you want, I can try doing some digging, but I think it's going to be
> a while before I can get to it, which is why I'm giving you as much info
> as I have (I really do not have much sim experience, but apparently we
> are short in reviewers for the sim code).

i'm not sure how the code today could possibly result in working behavior 
regardless of what is done in the autoconf stages.  the sim-traces configure 
option adds -DTRACE=0 or -DTRACE=1 to the command line.  the hw-properties.c 
then defines TRACE() after including everything.  so there is no chance for 
the -DTRACE logic to affect things here -- TRACE is always redefined from a 
0/1 value (or anything else) to a macro that expands into nothing.

if i had to divine intention, this is old code that was forgotten in the 
HW_TRACE() transition and so the correct fix would be:
	- delete the #define TRACE
	- change all TRACE call sites to HW_TRACE
	- change the non-existent "trace_devices" to "me"
this would have the code style match the rest of hw code in common/.

i didnt go this route originally because the sim/ dir is lacking in 
documentation, so i didnt want to break something inadvertently.  i'd rather 
have someone who knows the code ACK the idea.  but if the developer base for 
sim/ is as slim as the documentation, i'll be a bit more bold in my proposals 
based on my recent sim/ work for the Blackfin processor.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2] sim: avoid TRACE redefine warnings

[Mike Frysinger]

The common code sets up an autoconf option --enable-sim-trace which adds
-DTRACE= to CPPFLAGS.  This causes warnings in the building of some files
that declare a local TRACE() helper macro.  So punt it from hw-ports.c
(since it isn't actually used) and convert hw-properties.c to HW_TRACE().

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v2
	- convert hw-properties.c to HW_TRACE

2010-03-15  Mike Frysinger  <vapier@gentoo.org>

	* hw-ports.c (TRACE): Delete.
	* hw-properties.c (TRACE): Delete.
	(hw_find_ihandle_runtime_property): Change TRACE to HW_TRACE.
	(hw_find_integer_property): Likewise.
	(hw_find_integer_array_property): Likewise.
	(hw_add_duplicate_property): Likewise.

 sim/common/hw-ports.c      |    2 --
 sim/common/hw-properties.c |   18 ++++--------------
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/sim/common/hw-ports.c b/sim/common/hw-ports.c
index 8d5c14e..8f88cb3 100644
--- a/sim/common/hw-ports.c
+++ b/sim/common/hw-ports.c
@@ -35,8 +35,6 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include <ctype.h>
 
-#define TRACE(x,y)
-
 
 struct hw_port_edge {
   int my_port;
diff --git a/sim/common/hw-properties.c b/sim/common/hw-properties.c
index 3166e42..018a84c 100644
--- a/sim/common/hw-properties.c
+++ b/sim/common/hw-properties.c
@@ -33,8 +33,6 @@
 #endif
 #endif
 
-#define TRACE(A,B)
-
 /* property entries */
 
 struct hw_property_data {
@@ -418,9 +416,7 @@ hw_find_ihandle_runtime_property (struct hw *me,
 				  ihandle_runtime_property_spec *ihandle)
 {
   struct hw_property_data *entry = find_property_data (me, property);
-  TRACE (trace_devices,
-	 ("hw_find_ihandle_runtime_property(me=0x%lx, property=%s)\n",
-	  (long)me, property));
+  HW_TRACE ((me, "hw_find_ihandle_runtime_property(property=%s)\n", property));
   if (entry == NULL)
     hw_abort (me, "property \"%s\" not found", property);
   if (entry->property->type != ihandle_property
@@ -492,9 +488,7 @@ hw_find_integer_property (struct hw *me,
 {
   const struct hw_property *node;
   signed_cell integer;
-  TRACE (trace_devices,
-	 ("hw_find_integer(me=0x%lx, property=%s)\n",
-	  (long)me, property));
+  HW_TRACE ((me, "hw_find_integer(property=%s)\n", property));
   node = hw_find_property (me, property);
   if (node == NULL)
     hw_abort (me, "property \"%s\" not found", property);
@@ -514,9 +508,7 @@ hw_find_integer_array_property (struct hw *me,
   const struct hw_property *node;
   int sizeof_integer = sizeof (*integer);
   signed_cell *cell;
-  TRACE (trace_devices,
-	 ("hw_find_integer(me=0x%lx, property=%s)\n",
-	  (long)me, property));
+  HW_TRACE ((me, "hw_find_integer(property=%s)\n", property));
   
   /* check things sane */
   node = hw_find_property (me, property);
@@ -886,9 +878,7 @@ hw_add_duplicate_property (struct hw *me,
 			   const struct hw_property *original)
 {
   struct hw_property_data *master;
-  TRACE (trace_devices,
-	 ("hw_add_duplicate_property(me=0x%lx, property=%s, ...)\n",
-	  (long)me, property));
+  HW_TRACE ((me, "hw_add_duplicate_property(property=%s, ...)\n", property));
   if (original->disposition != permenant_object)
     hw_abort (me, "Can only duplicate permenant objects");
   /* find the original's master */
-- 
1.7.0

Re: [PATCH v2] sim: avoid TRACE redefine warnings

[Joel Brobecker]

> 2010-03-15  Mike Frysinger  <vapier@gentoo.org>
> 
> 	* hw-ports.c (TRACE): Delete.
> 	* hw-properties.c (TRACE): Delete.
> 	(hw_find_ihandle_runtime_property): Change TRACE to HW_TRACE.
> 	(hw_find_integer_property): Likewise.
> 	(hw_find_integer_array_property): Likewise.
> 	(hw_add_duplicate_property): Likewise.

This looks good to me. I don't understand why people use two parentheses
in the call to HW_TRACE:

    HW_TRACE ((me, [...], property))

But it looks like it's the style being used, so OK.

-- 
Joel

Re: [PATCH v2] sim: avoid TRACE redefine warnings

[Doug Evans]

On Tue, Mar 16, 2010 at 9:06 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> 2010-03-15  Mike Frysinger  <vapier@gentoo.org>
>>
>>       * hw-ports.c (TRACE): Delete.
>>       * hw-properties.c (TRACE): Delete.
>>       (hw_find_ihandle_runtime_property): Change TRACE to HW_TRACE.
>>       (hw_find_integer_property): Likewise.
>>       (hw_find_integer_array_property): Likewise.
>>       (hw_add_duplicate_property): Likewise.
>
> This looks good to me. I don't understand why people use two parentheses
> in the call to HW_TRACE:
>
>    HW_TRACE ((me, [...], property))
>
> But it looks like it's the style being used, so OK.

For reference sake,
It's not style, it's correctness.
Having two parens  allows for variadic macros in the absence of
explicit support from the language.

void hw_trace
(struct hw *me,
 const char *fmt,
 ...) __attribute__ ((format (printf, 2, 3)));

#define HW_TRACE(ARGS) \
do { \
  if (hw_trace_p (me)) \
    { \
      hw_trace ARGS; \
    } \
} while (0)