New ARI warning Sat Jan 18 01:52:44 UTC 2014

New ARI warning Sat Jan 18 01:52:44 UTC 2014

[GDB Administrator]

90a91
> gdb/common/common-utils.h:37: regression: __func__: Do not use __func__, ISO C 90 does not support this macro
gdb/common/common-utils.h:37:#define FUNCTION_NAME		__func__

[PATCH 1/2] Expand documentation of common-utils.h::FUNCTION_NAME

[Joel Brobecker]

Hello,

While looking at this macro, I noticed that it wasn't always necessarily
defined. That prompted me to search the current sources to make sure
that all uses were adequately protected, which they were. But to help
prevent future uses to be made unprotected, this patch expands the
current macro documentation a bit.

gdb/ChangeLog:

        * common/common-utils.h (FUNCTION_NAME): Expand the macro's
        documentation a bit.

I would commit on its own, but since I am going to put the next in
for the same macro up for review, it's just as easy to make that one
wait as well, in case there are comments.

Thanks,
-- 
Joel

---
 gdb/common/common-utils.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 2d99549..5960c55 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -25,7 +25,12 @@
 #include <stddef.h>
 #include <stdarg.h>
 
-/* Version 2.4 and later of GCC define a magical variable `__PRETTY_FUNCTION__'
+/* If possible, define FUNCTION_NAME, a macro containing the name of
+   the function being defined.  Since this macro may not always be
+   defined, all uses must be protected by appropriate macro definition
+   checks (Eg: "#ifdef FUNCTION_NAME").
+
+   Version 2.4 and later of GCC define a magical variable `__PRETTY_FUNCTION__'
    which contains the name of the function currently being defined.
    This is broken in G++ before version 2.6.
    C9x has a similar variable called __func__, but prefer the GCC one since
-- 
1.8.3.2

[PATCH 2/2] Add ARI (ok) marker for __func__ reference in common-utils.h

[Joel Brobecker]

Hello,

The ARI script flagged the use of the __func__ variable, which
is normally not allowed (not defined in C90). However, this particular
use is OK, as the reference is only made when __STDC_VERSION__ >=
199901L.  So, add an "ARI:" comment to explicitly OK this use.

gdb/ChangeLog:

        * common/common-utils.h: Add "ARI:" comment besides __func__
        reference.

OK to commit?

Thanks,
-- 
Joel

---
 gdb/common/common-utils.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 5960c55..063698d 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -39,7 +39,7 @@
 #define FUNCTION_NAME		__PRETTY_FUNCTION__
 #else
 #if defined __STDC_VERSION__ && __STDC_VERSION__ >= 199901L
-#define FUNCTION_NAME		__func__
+#define FUNCTION_NAME		__func__  /* ARI: func */
 #endif
 #endif
 
-- 
1.8.3.2

Re: [PATCH 1/2] Expand documentation of common-utils.h::FUNCTION_NAME

[Doug Evans]

On Tue, Jan 21, 2014 at 2:36 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> Hello,
>
> While looking at this macro, I noticed that it wasn't always necessarily
> defined. That prompted me to search the current sources to make sure
> that all uses were adequately protected, which they were. But to help
> prevent future uses to be made unprotected, this patch expands the
> current macro documentation a bit.
>
> gdb/ChangeLog:
>
>         * common/common-utils.h (FUNCTION_NAME): Expand the macro's
>         documentation a bit.
>
> I would commit on its own, but since I am going to put the next in
> for the same macro up for review, it's just as easy to make that one
> wait as well, in case there are comments.

Yeah, I stumbled a bit on this myself.
It's not clear to me whether not defining it or defining it as NULL
(and update all current users to deal with that) is better but I went
with keeping things as they are.

Re: [PATCH 2/2] Add ARI (ok) marker for __func__ reference in common-utils.h

[Doug Evans]

On Tue, Jan 21, 2014 at 2:36 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> Hello,
>
> The ARI script flagged the use of the __func__ variable, which
> is normally not allowed (not defined in C90). However, this particular
> use is OK, as the reference is only made when __STDC_VERSION__ >=
> 199901L.  So, add an "ARI:" comment to explicitly OK this use.
>
> gdb/ChangeLog:
>
>         * common/common-utils.h: Add "ARI:" comment besides __func__
>         reference.
>
> OK to commit?

Fine by me.

English nit:  "... comment beside __func__ reference".

I wonder though how ARI handled gdb_assert.h (where FUNCTION_NAME
originates from).

Re: [PATCH 2/2] Add ARI (ok) marker for __func__ reference in common-utils.h

[Joel Brobecker]

> > gdb/ChangeLog:
> >
> >         * common/common-utils.h: Add "ARI:" comment besides __func__
> >         reference.
> >
> > OK to commit?
> 
> Fine by me.

Thanks!


> English nit:  "... comment beside __func__ reference".
> 
> I wonder though how ARI handled gdb_assert.h (where FUNCTION_NAME
> originates from).

It probably did not - it just became visible again because of
the move (as I understand it, we diff the report with the report
from the previous day, and if we find any additions, we send
them via email, and "diff" is not sophisticated enough to detect
moves).

-- 
Joel

Re: [PATCH 1/2] Expand documentation of common-utils.h::FUNCTION_NAME

[Joel Brobecker]

> > gdb/ChangeLog:
> >
> >         * common/common-utils.h (FUNCTION_NAME): Expand the macro's
> >         documentation a bit.
> >
> > I would commit on its own, but since I am going to put the next in
> > for the same macro up for review, it's just as easy to make that one
> > wait as well, in case there are comments.
> 
> Yeah, I stumbled a bit on this myself.
> It's not clear to me whether not defining it or defining it as NULL
> (and update all current users to deal with that) is better but I went
> with keeping things as they are.

I almost had the same thoughts. I agree that it's just best to let
things as they are until we have evidence that changing them would
be beneficial. The difference is that I was thinking of defining
FUNCTION_NAME to something like "<unknown function>" rather than NULL.
Without more evidence, not clear which would be best...

-- 
Joel

pushed: [PATCH 2/2] Add ARI (ok) marker for __func__ reference in common-utils.h

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * common/common-utils.h: Add "ARI:" comment beside __func__
>         reference.

This patch is now in, with the English correction mentioned by Doug.

-- 
Joel

pushed: [PATCH 1/2] Expand documentation of common-utils.h::FUNCTION_NAME

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * common/common-utils.h (FUNCTION_NAME): Expand the macro's
>         documentation a bit.

FTR: This patch is now in.

-- 
Joel

Re: [PATCH 1/2] Expand documentation of common-utils.h::FUNCTION_NAME

[Doug Evans]

On Tue, Jan 21, 2014 at 8:59 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> > gdb/ChangeLog:
>> >
>> >         * common/common-utils.h (FUNCTION_NAME): Expand the macro's
>> >         documentation a bit.
>> >
>> > I would commit on its own, but since I am going to put the next in
>> > for the same macro up for review, it's just as easy to make that one
>> > wait as well, in case there are comments.
>>
>> Yeah, I stumbled a bit on this myself.
>> It's not clear to me whether not defining it or defining it as NULL
>> (and update all current users to deal with that) is better but I went
>> with keeping things as they are.
>
> I almost had the same thoughts. I agree that it's just best to let
> things as they are until we have evidence that changing them would
> be beneficial. The difference is that I was thinking of defining
> FUNCTION_NAME to something like "<unknown function>" rather than NULL.
> Without more evidence, not clear which would be best...

Yeah.
The argument against <unknown function> is that maybe sometime one
would want to know if its unknown, and comparison with NULL is easier,
more maintainable than strcmp (unless "<unknown function>" was a
macro, but maybe that's overkill).