All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow)
@ 2012-06-15 12:04 Henrique Dante de Almeida
  2012-06-15 12:29 ` Anderson Lizardo
  2012-06-15 15:27 ` Gustavo Padovan
  0 siblings, 2 replies; 11+ messages in thread
From: Henrique Dante de Almeida @ 2012-06-15 12:04 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Henrique Dante de Almeida

---
 gdbus/object.c |   39 +++++++++++++++++++++------------------
 gdbus/watch.c  |    4 ++--
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/gdbus/object.c b/gdbus/object.c
index 900e7ab..ff52ead 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -79,7 +79,7 @@ static void print_arguments(GString *gstr, const GDBusArgInfo *args,
 static void generate_interface_xml(GString *gstr, struct interface_data *iface)
 {
 	const GDBusMethodTable *method;
-	const GDBusSignalTable *signal;
+	const GDBusSignalTable *signal_entry;
 
 	for (method = iface->methods; method && method->name; method++) {
 		gboolean deprecated = method->flags &
@@ -108,17 +108,19 @@ static void generate_interface_xml(GString *gstr, struct interface_data *iface)
 		}
 	}
 
-	for (signal = iface->signals; signal && signal->name; signal++) {
-		gboolean deprecated = signal->flags &
+	for (signal_entry = iface->signals; signal_entry && signal_entry->name;
+							signal_entry++) {
+		gboolean deprecated = signal_entry->flags &
 						G_DBUS_SIGNAL_FLAG_DEPRECATED;
 
-		if (!deprecated && !(signal->args && signal->args->name))
+		if (!deprecated && !(signal_entry->args &&
+						signal_entry->args->name))
 			g_string_append_printf(gstr, "\t\t<signal name=\"%s\"/>\n",
-								signal->name);
+							signal_entry->name);
 		else {
 			g_string_append_printf(gstr, "\t\t<signal name=\"%s\">\n",
-								signal->name);
-			print_arguments(gstr, signal->args, NULL);
+							signal_entry->name);
+			print_arguments(gstr, signal_entry->args, NULL);
 
 			if (deprecated)
 				g_string_append_printf(gstr, "\t\t\t<annotation name=\"org.freedesktop.DBus.Deprecated\" value=\"true\"/>\n");
@@ -592,7 +594,7 @@ static gboolean check_signal(DBusConnection *conn, const char *path,
 {
 	struct generic_data *data = NULL;
 	struct interface_data *iface;
-	const GDBusSignalTable *signal;
+	const GDBusSignalTable *signal_entry;
 
 	*args = NULL;
 	if (!dbus_connection_get_object_path_data(conn, path,
@@ -609,9 +611,10 @@ static gboolean check_signal(DBusConnection *conn, const char *path,
 		return FALSE;
 	}
 
-	for (signal = iface->signals; signal && signal->name; signal++) {
-		if (!strcmp(signal->name, name)) {
-			*args = signal->args;
+	for (signal_entry = iface->signals; signal_entry && signal_entry->name;
+							signal_entry++) {
+		if (!strcmp(signal_entry->name, name)) {
+			*args = signal_entry->args;
 			return TRUE;
 		}
 	}
@@ -627,34 +630,34 @@ static dbus_bool_t emit_signal_valist(DBusConnection *conn,
 						int first,
 						va_list var_args)
 {
-	DBusMessage *signal;
+	DBusMessage *message;
 	dbus_bool_t ret;
 	const GDBusArgInfo *args;
 
 	if (!check_signal(conn, path, interface, name, &args))
 		return FALSE;
 
-	signal = dbus_message_new_signal(path, interface, name);
-	if (signal == NULL) {
+	message = dbus_message_new_signal(path, interface, name);
+	if (message == NULL) {
 		error("Unable to allocate new %s.%s signal", interface,  name);
 		return FALSE;
 	}
 
-	ret = dbus_message_append_args_valist(signal, first, var_args);
+	ret = dbus_message_append_args_valist(message, first, var_args);
 	if (!ret)
 		goto fail;
 
-	if (g_dbus_args_have_signature(args, signal) == FALSE) {
+	if (g_dbus_args_have_signature(args, message) == FALSE) {
 		error("%s.%s: expected signature'%s' but got '%s'",
 				interface, name, args, signature);
 		ret = FALSE;
 		goto fail;
 	}
 
-	ret = dbus_connection_send(conn, signal, NULL);
+	ret = dbus_connection_send(conn, message, NULL);
 
 fail:
-	dbus_message_unref(signal);
+	dbus_message_unref(message);
 
 	return ret;
 }
diff --git a/gdbus/watch.c b/gdbus/watch.c
index 9a716b0..4be412c 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -305,7 +305,7 @@ static struct filter_callback *filter_data_add_callback(
 						struct filter_data *data,
 						GDBusWatchFunction connect,
 						GDBusWatchFunction disconnect,
-						GDBusSignalFunction signal,
+						GDBusSignalFunction signal_func,
 						GDBusDestroyFunction destroy,
 						void *user_data)
 {
@@ -315,7 +315,7 @@ static struct filter_callback *filter_data_add_callback(
 
 	cb->conn_func = connect;
 	cb->disc_func = disconnect;
-	cb->signal_func = signal;
+	cb->signal_func = signal_func;
 	cb->destroy_func = destroy;
 	cb->user_data = user_data;
 	cb->id = ++listener_id;
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow)
  2012-06-15 12:04 [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow) Henrique Dante de Almeida
@ 2012-06-15 12:29 ` Anderson Lizardo
  2012-06-15 12:43   ` Henrique Dante
  2012-06-15 15:27 ` Gustavo Padovan
  1 sibling, 1 reply; 11+ messages in thread
From: Anderson Lizardo @ 2012-06-15 12:29 UTC (permalink / raw)
  To: Henrique Dante de Almeida; +Cc: linux-bluetooth

Hi Henrique,

On Fri, Jun 15, 2012 at 8:04 AM, Henrique Dante de Almeida
<hdante@profusion.mobi> wrote:
> ---
>  gdbus/object.c |   39 +++++++++++++++++++++------------------
>  gdbus/watch.c  |    4 ++--
>  2 files changed, 23 insertions(+), 20 deletions(-)

Would it be interesting to add this option to acinclude.m4? Or does it
generate too much noise?

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow)
  2012-06-15 12:29 ` Anderson Lizardo
@ 2012-06-15 12:43   ` Henrique Dante
  2012-06-15 12:45     ` Henrique Dante
  0 siblings, 1 reply; 11+ messages in thread
From: Henrique Dante @ 2012-06-15 12:43 UTC (permalink / raw)
  To: Anderson Lizardo; +Cc: linux-bluetooth

On Fri, Jun 15, 2012 at 9:29 AM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> Hi Henrique,
>
> On Fri, Jun 15, 2012 at 8:04 AM, Henrique Dante de Almeida
> <hdante@profusion.mobi> wrote:
>> ---
>>  gdbus/object.c |   39 +++++++++++++++++++++------------------
>>  gdbus/watch.c  |    4 ++--
>>  2 files changed, 23 insertions(+), 20 deletions(-)
>
> Would it be interesting to add this option to acinclude.m4? Or does it
> generate too much noise?

 It generates few warnings. Depending on the acceptance of this patch,
I could fix bluez as a whole and add -Wshadow to acinclude.m4.

>
> Regards,
> --
> Anderson Lizardo
> Instituto Nokia de Tecnologia - INdT
> Manaus - Brazil

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow)
  2012-06-15 12:43   ` Henrique Dante
@ 2012-06-15 12:45     ` Henrique Dante
  2012-06-19 18:29       ` Joao Paulo Rechi Vita
  0 siblings, 1 reply; 11+ messages in thread
From: Henrique Dante @ 2012-06-15 12:45 UTC (permalink / raw)
  To: Anderson Lizardo; +Cc: linux-bluetooth

On Fri, Jun 15, 2012 at 9:43 AM, Henrique Dante <hdante@profusion.mobi> wrote:
> On Fri, Jun 15, 2012 at 9:29 AM, Anderson Lizardo
> <anderson.lizardo@openbossa.org> wrote:
>> Hi Henrique,
>>
>> On Fri, Jun 15, 2012 at 8:04 AM, Henrique Dante de Almeida
>> <hdante@profusion.mobi> wrote:
>>> ---
>>>  gdbus/object.c |   39 +++++++++++++++++++++------------------
>>>  gdbus/watch.c  |    4 ++--
>>>  2 files changed, 23 insertions(+), 20 deletions(-)
>>
>> Would it be interesting to add this option to acinclude.m4? Or does it
>> generate too much noise?
>
>  It generates few warnings. Depending on the acceptance of this patch,
> I could fix bluez as a whole and add -Wshadow to acinclude.m4.

 Actually, I had a partial build here. Ignore the previous answer, it
generates a lot of warnings.

>
>>
>> Regards,
>> --
>> Anderson Lizardo
>> Instituto Nokia de Tecnologia - INdT
>> Manaus - Brazil

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow)
  2012-06-15 12:04 [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow) Henrique Dante de Almeida
  2012-06-15 12:29 ` Anderson Lizardo
@ 2012-06-15 15:27 ` Gustavo Padovan
  1 sibling, 0 replies; 11+ messages in thread
From: Gustavo Padovan @ 2012-06-15 15:27 UTC (permalink / raw)
  To: Henrique Dante de Almeida; +Cc: linux-bluetooth

Hi Henrique,

* Henrique Dante de Almeida <hdante@profusion.mobi> [2012-06-15 09:04:08 -0300]:

> ---
>  gdbus/object.c |   39 +++++++++++++++++++++------------------
>  gdbus/watch.c  |    4 ++--
>  2 files changed, 23 insertions(+), 20 deletions(-)

Please keep the subject line of your patch with less than 72 chars. You could
split it like:

gdbus: Rename variables named "signal"

so that it can be compiled with -Wshadow

	Gustavo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow)
  2012-06-15 12:45     ` Henrique Dante
@ 2012-06-19 18:29       ` Joao Paulo Rechi Vita
  2012-06-19 21:41         ` Lucas De Marchi
  0 siblings, 1 reply; 11+ messages in thread
From: Joao Paulo Rechi Vita @ 2012-06-19 18:29 UTC (permalink / raw)
  To: Henrique Dante; +Cc: Anderson Lizardo, linux-bluetooth

On Fri, Jun 15, 2012 at 9:45 AM, Henrique Dante <hdante@profusion.mobi> wrote:
> On Fri, Jun 15, 2012 at 9:43 AM, Henrique Dante <hdante@profusion.mobi> wrote:
>> On Fri, Jun 15, 2012 at 9:29 AM, Anderson Lizardo
>> <anderson.lizardo@openbossa.org> wrote:
>>> Hi Henrique,
>>>
>>> On Fri, Jun 15, 2012 at 8:04 AM, Henrique Dante de Almeida
>>> <hdante@profusion.mobi> wrote:
>>>> ---
>>>>  gdbus/object.c |   39 +++++++++++++++++++++------------------
>>>>  gdbus/watch.c  |    4 ++--
>>>>  2 files changed, 23 insertions(+), 20 deletions(-)
>>>
>>> Would it be interesting to add this option to acinclude.m4? Or does it
>>> generate too much noise?
>>
>>  It generates few warnings. Depending on the acceptance of this patch,
>> I could fix bluez as a whole and add -Wshadow to acinclude.m4.
>
>  Actually, I had a partial build here. Ignore the previous answer, it
> generates a lot of warnings.
>

If we're not going to enable -Wshadow by default, does it make sense
to apply this patch? Who is going to check if no new shadow warnings
are being inserted in new commits?

-- 
João Paulo Rechi Vita
Openbossa Labs - INdT

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow)
  2012-06-19 18:29       ` Joao Paulo Rechi Vita
@ 2012-06-19 21:41         ` Lucas De Marchi
  2012-06-20  9:41           ` Johan Hedberg
  2012-06-20 13:20           ` Joao Paulo Rechi Vita
  0 siblings, 2 replies; 11+ messages in thread
From: Lucas De Marchi @ 2012-06-19 21:41 UTC (permalink / raw)
  To: Joao Paulo Rechi Vita; +Cc: Henrique Dante, Anderson Lizardo, linux-bluetooth

On Tue, Jun 19, 2012 at 3:29 PM, Joao Paulo Rechi Vita
<jprvita@openbossa.org> wrote:
> On Fri, Jun 15, 2012 at 9:45 AM, Henrique Dante <hdante@profusion.mobi> wrote:
>> On Fri, Jun 15, 2012 at 9:43 AM, Henrique Dante <hdante@profusion.mobi> wrote:
>>> On Fri, Jun 15, 2012 at 9:29 AM, Anderson Lizardo
>>> <anderson.lizardo@openbossa.org> wrote:
>>>> Hi Henrique,
>>>>
>>>> On Fri, Jun 15, 2012 at 8:04 AM, Henrique Dante de Almeida
>>>> <hdante@profusion.mobi> wrote:
>>>>> ---
>>>>>  gdbus/object.c |   39 +++++++++++++++++++++------------------
>>>>>  gdbus/watch.c  |    4 ++--
>>>>>  2 files changed, 23 insertions(+), 20 deletions(-)
>>>>
>>>> Would it be interesting to add this option to acinclude.m4? Or does it
>>>> generate too much noise?
>>>
>>>  It generates few warnings. Depending on the acceptance of this patch,
>>> I could fix bluez as a whole and add -Wshadow to acinclude.m4.
>>
>>  Actually, I had a partial build here. Ignore the previous answer, it
>> generates a lot of warnings.
>>
>
> If we're not going to enable -Wshadow by default, does it make sense
> to apply this patch? Who is going to check if no new shadow warnings
> are being inserted in new commits?

I'm all for doing the following:

1) Fix all the places with shadow variables
2) Add -Wshadow to the warning flags

There are lots of them.


Lucas De Marchi

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow)
  2012-06-19 21:41         ` Lucas De Marchi
@ 2012-06-20  9:41           ` Johan Hedberg
  2012-06-20 13:20           ` Joao Paulo Rechi Vita
  1 sibling, 0 replies; 11+ messages in thread
From: Johan Hedberg @ 2012-06-20  9:41 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Joao Paulo Rechi Vita, Henrique Dante, Anderson Lizardo, linux-bluetooth

Hi Lucas,

On Tue, Jun 19, 2012, Lucas De Marchi wrote:
> >>>> Would it be interesting to add this option to acinclude.m4? Or does it
> >>>> generate too much noise?
> >>>
> >>>  It generates few warnings. Depending on the acceptance of this patch,
> >>> I could fix bluez as a whole and add -Wshadow to acinclude.m4.
> >>
> >>  Actually, I had a partial build here. Ignore the previous answer, it
> >> generates a lot of warnings.
> >>
> >
> > If we're not going to enable -Wshadow by default, does it make sense
> > to apply this patch? Who is going to check if no new shadow warnings
> > are being inserted in new commits?
> 
> I'm all for doing the following:
> 
> 1) Fix all the places with shadow variables
> 2) Add -Wshadow to the warning flags
> 
> There are lots of them.

Agreed. This could be one of the targets before releasing BlueZ 5.0.

Johan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow)
  2012-06-19 21:41         ` Lucas De Marchi
  2012-06-20  9:41           ` Johan Hedberg
@ 2012-06-20 13:20           ` Joao Paulo Rechi Vita
  2012-06-20 13:59             ` Joao Paulo Rechi Vita
  1 sibling, 1 reply; 11+ messages in thread
From: Joao Paulo Rechi Vita @ 2012-06-20 13:20 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: Henrique Dante, Anderson Lizardo, linux-bluetooth

On Tue, Jun 19, 2012 at 6:41 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> On Tue, Jun 19, 2012 at 3:29 PM, Joao Paulo Rechi Vita
> <jprvita@openbossa.org> wrote:
>> On Fri, Jun 15, 2012 at 9:45 AM, Henrique Dante <hdante@profusion.mobi> wrote:
>>> On Fri, Jun 15, 2012 at 9:43 AM, Henrique Dante <hdante@profusion.mobi> wrote:
>>>> On Fri, Jun 15, 2012 at 9:29 AM, Anderson Lizardo
>>>> <anderson.lizardo@openbossa.org> wrote:
>>>>> Hi Henrique,
>>>>>
>>>>> On Fri, Jun 15, 2012 at 8:04 AM, Henrique Dante de Almeida
>>>>> <hdante@profusion.mobi> wrote:
>>>>>> ---
>>>>>>  gdbus/object.c |   39 +++++++++++++++++++++------------------
>>>>>>  gdbus/watch.c  |    4 ++--
>>>>>>  2 files changed, 23 insertions(+), 20 deletions(-)
>>>>>
>>>>> Would it be interesting to add this option to acinclude.m4? Or does it
>>>>> generate too much noise?
>>>>
>>>>  It generates few warnings. Depending on the acceptance of this patch,
>>>> I could fix bluez as a whole and add -Wshadow to acinclude.m4.
>>>
>>>  Actually, I had a partial build here. Ignore the previous answer, it
>>> generates a lot of warnings.
>>>
>>
>> If we're not going to enable -Wshadow by default, does it make sense
>> to apply this patch? Who is going to check if no new shadow warnings
>> are being inserted in new commits?
>
> I'm all for doing the following:
>
> 1) Fix all the places with shadow variables
> 2) Add -Wshadow to the warning flags
>
> There are lots of them.
>

Yes, that makes sense.

-- 
João Paulo Rechi Vita
Openbossa Labs - INdT

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow)
  2012-06-20 13:20           ` Joao Paulo Rechi Vita
@ 2012-06-20 13:59             ` Joao Paulo Rechi Vita
  2012-06-22 21:10               ` Henrique Dante
  0 siblings, 1 reply; 11+ messages in thread
From: Joao Paulo Rechi Vita @ 2012-06-20 13:59 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Henrique Dante, Anderson Lizardo, linux-bluetooth, Johan Hedberg

On Wed, Jun 20, 2012 at 10:20 AM, Joao Paulo Rechi Vita
<jprvita@openbossa.org> wrote:
> On Tue, Jun 19, 2012 at 6:41 PM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> On Tue, Jun 19, 2012 at 3:29 PM, Joao Paulo Rechi Vita
>> <jprvita@openbossa.org> wrote:
>>> On Fri, Jun 15, 2012 at 9:45 AM, Henrique Dante <hdante@profusion.mobi> wrote:
>>>> On Fri, Jun 15, 2012 at 9:43 AM, Henrique Dante <hdante@profusion.mobi> wrote:
>>>>> On Fri, Jun 15, 2012 at 9:29 AM, Anderson Lizardo
>>>>> <anderson.lizardo@openbossa.org> wrote:
>>>>>> Hi Henrique,
>>>>>>
>>>>>> On Fri, Jun 15, 2012 at 8:04 AM, Henrique Dante de Almeida
>>>>>> <hdante@profusion.mobi> wrote:
>>>>>>> ---
>>>>>>>  gdbus/object.c |   39 +++++++++++++++++++++------------------
>>>>>>>  gdbus/watch.c  |    4 ++--
>>>>>>>  2 files changed, 23 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> Would it be interesting to add this option to acinclude.m4? Or does it
>>>>>> generate too much noise?
>>>>>
>>>>>  It generates few warnings. Depending on the acceptance of this patch,
>>>>> I could fix bluez as a whole and add -Wshadow to acinclude.m4.
>>>>
>>>>  Actually, I had a partial build here. Ignore the previous answer, it
>>>> generates a lot of warnings.
>>>>
>>>
>>> If we're not going to enable -Wshadow by default, does it make sense
>>> to apply this patch? Who is going to check if no new shadow warnings
>>> are being inserted in new commits?
>>
>> I'm all for doing the following:
>>
>> 1) Fix all the places with shadow variables
>> 2) Add -Wshadow to the warning flags
>>
>> There are lots of them.
>>
>
> Yes, that makes sense.
>

Or not completely, as researching a little bit on what kind of
warnings -Wshadow generates shows that not all of them are useful [1]
(I'm quoting the relevant part of this link below). I think before
getting our hands dirty to change this, we should look if we want to
live with all the restrictions imposed by it. Opinions?

[1] http://kerneltrap.org/node/7434

"""
From: Linus Torvalds [email blocked]
Subject: Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()
Date:	Tue, 28 Nov 2006 16:13:05 -0800 (PST)



On Wed, 29 Nov 2006, Jesper Juhl wrote:
>
> I would venture that "-Wshadow" is another one of those.

I'd agree, except for the fact that gcc does a horribly _bad_ job of
-Wshadow, making it (again) totally unusable.

For example, it's often entirely interesting to hear about local variables
that shadow each other. No question about it.

HOWEVER. It's _not_ really interesting to hear about a local variable that
happens to have a common name that is also shared by a extern function.

There just isn't any room for confusion, and it's actually not even that
unusual - I tried using -Wshadow on real programs, and it was just
horribly irritating.

In the kernel, we had obvious things like local use of "jiffies" that just
make _total_ sense in a small inline function, and the fact that there
happens to be an extern declaration for "jiffies" just isn't very
interesting.

Similarly, with nested macro expansion, even the "local variable shadows
another local variable" case - that looks like it should have an obvious
warning on the face of it - really isn't always necessarily that
interesting after all. Maybe it is a bug, maybe it isn't, but it's no
longer _obviously_ bogus any more.

So I'm not convinced about the usefulness of "-Wshadow". ESPECIALLY the
way that gcc implements it, it's almost totally useless in real life.

For example, I tried it on "git" one time, and this is a perfect example
of why "-Wshadow" is totally broken:

	diff-delta.c: In function 'create_delta_index':
	diff-delta.c:142: warning: declaration of 'index' shadows a global declaration

(and there's a _lot_ of those). If I'm not allowed to use "index" as a
local variable and include <string.h> at the same time, something is
simply SERIOUSLY WRONG with the warning.

So the fact is, the C language has scoping rules for a reason. Can you
screw yourself by usign them badly? Sure. But that does NOT mean that the
same name in different scopes is a bad thing that should be warned about.

If I wanted a language that didn't allow me to do anything wrong, I'd be
using Pascal. As it is, it turns out that things that "look" wrong on a
local level are often not wrong after all.

			Linus
"""

-- 
João Paulo Rechi Vita
Openbossa Labs - INdT

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow)
  2012-06-20 13:59             ` Joao Paulo Rechi Vita
@ 2012-06-22 21:10               ` Henrique Dante
  0 siblings, 0 replies; 11+ messages in thread
From: Henrique Dante @ 2012-06-22 21:10 UTC (permalink / raw)
  To: Joao Paulo Rechi Vita
  Cc: Lucas De Marchi, Anderson Lizardo, linux-bluetooth, Johan Hedberg

 Any conclusions about this ?

On Wed, Jun 20, 2012 at 10:59 AM, Joao Paulo Rechi Vita
<jprvita@openbossa.org> wrote:
> On Wed, Jun 20, 2012 at 10:20 AM, Joao Paulo Rechi Vita
> <jprvita@openbossa.org> wrote:
>> On Tue, Jun 19, 2012 at 6:41 PM, Lucas De Marchi
>> <lucas.demarchi@profusion.mobi> wrote:
>>> On Tue, Jun 19, 2012 at 3:29 PM, Joao Paulo Rechi Vita
>>> <jprvita@openbossa.org> wrote:
>>>> On Fri, Jun 15, 2012 at 9:45 AM, Henrique Dante <hdante@profusion.mobi> wrote:
>>>>> On Fri, Jun 15, 2012 at 9:43 AM, Henrique Dante <hdante@profusion.mobi> wrote:
>>>>>> On Fri, Jun 15, 2012 at 9:29 AM, Anderson Lizardo
>>>>>> <anderson.lizardo@openbossa.org> wrote:
>>>>>>> Hi Henrique,
>>>>>>>
>>>>>>> On Fri, Jun 15, 2012 at 8:04 AM, Henrique Dante de Almeida
>>>>>>> <hdante@profusion.mobi> wrote:
>>>>>>>> ---
>>>>>>>>  gdbus/object.c |   39 +++++++++++++++++++++------------------
>>>>>>>>  gdbus/watch.c  |    4 ++--
>>>>>>>>  2 files changed, 23 insertions(+), 20 deletions(-)
>>>>>>>
>>>>>>> Would it be interesting to add this option to acinclude.m4? Or does it
>>>>>>> generate too much noise?
>>>>>>
>>>>>>  It generates few warnings. Depending on the acceptance of this patch,
>>>>>> I could fix bluez as a whole and add -Wshadow to acinclude.m4.
>>>>>
>>>>>  Actually, I had a partial build here. Ignore the previous answer, it
>>>>> generates a lot of warnings.
>>>>>
>>>>
>>>> If we're not going to enable -Wshadow by default, does it make sense
>>>> to apply this patch? Who is going to check if no new shadow warnings
>>>> are being inserted in new commits?
>>>
>>> I'm all for doing the following:
>>>
>>> 1) Fix all the places with shadow variables
>>> 2) Add -Wshadow to the warning flags
>>>
>>> There are lots of them.
>>>
>>
>> Yes, that makes sense.
>>
>
> Or not completely, as researching a little bit on what kind of
> warnings -Wshadow generates shows that not all of them are useful [1]
> (I'm quoting the relevant part of this link below). I think before
> getting our hands dirty to change this, we should look if we want to
> live with all the restrictions imposed by it. Opinions?
>
> [1] http://kerneltrap.org/node/7434
>
> """
> From: Linus Torvalds [email blocked]
> Subject: Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()
> Date:   Tue, 28 Nov 2006 16:13:05 -0800 (PST)
>
>
>
> On Wed, 29 Nov 2006, Jesper Juhl wrote:
>>
>> I would venture that "-Wshadow" is another one of those.
>
> I'd agree, except for the fact that gcc does a horribly _bad_ job of
> -Wshadow, making it (again) totally unusable.
>
> For example, it's often entirely interesting to hear about local variables
> that shadow each other. No question about it.
>
> HOWEVER. It's _not_ really interesting to hear about a local variable that
> happens to have a common name that is also shared by a extern function.
>
> There just isn't any room for confusion, and it's actually not even that
> unusual - I tried using -Wshadow on real programs, and it was just
> horribly irritating.
>
> In the kernel, we had obvious things like local use of "jiffies" that just
> make _total_ sense in a small inline function, and the fact that there
> happens to be an extern declaration for "jiffies" just isn't very
> interesting.
>
> Similarly, with nested macro expansion, even the "local variable shadows
> another local variable" case - that looks like it should have an obvious
> warning on the face of it - really isn't always necessarily that
> interesting after all. Maybe it is a bug, maybe it isn't, but it's no
> longer _obviously_ bogus any more.
>
> So I'm not convinced about the usefulness of "-Wshadow". ESPECIALLY the
> way that gcc implements it, it's almost totally useless in real life.
>
> For example, I tried it on "git" one time, and this is a perfect example
> of why "-Wshadow" is totally broken:
>
>        diff-delta.c: In function 'create_delta_index':
>        diff-delta.c:142: warning: declaration of 'index' shadows a global declaration
>
> (and there's a _lot_ of those). If I'm not allowed to use "index" as a
> local variable and include <string.h> at the same time, something is
> simply SERIOUSLY WRONG with the warning.
>
> So the fact is, the C language has scoping rules for a reason. Can you
> screw yourself by usign them badly? Sure. But that does NOT mean that the
> same name in different scopes is a bad thing that should be warned about.
>
> If I wanted a language that didn't allow me to do anything wrong, I'd be
> using Pascal. As it is, it turns out that things that "look" wrong on a
> local level are often not wrong after all.
>
>                        Linus
> """
>
> --
> João Paulo Rechi Vita
> Openbossa Labs - INdT

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-06-22 21:10 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-15 12:04 [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow) Henrique Dante de Almeida
2012-06-15 12:29 ` Anderson Lizardo
2012-06-15 12:43   ` Henrique Dante
2012-06-15 12:45     ` Henrique Dante
2012-06-19 18:29       ` Joao Paulo Rechi Vita
2012-06-19 21:41         ` Lucas De Marchi
2012-06-20  9:41           ` Johan Hedberg
2012-06-20 13:20           ` Joao Paulo Rechi Vita
2012-06-20 13:59             ` Joao Paulo Rechi Vita
2012-06-22 21:10               ` Henrique Dante
2012-06-15 15:27 ` Gustavo Padovan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.