[PATCH] lib: fix warnings when building with clang

[PATCH] lib: fix warnings when building with clang

[Jani Nikula]

Building notmuch with CC=clang and CXX=clang++ produces the warnings:

CC -O2 lib/tags.o
lib/tags.c:43:5: warning: expression result unused [-Wunused-value]
    talloc_steal (tags, list);
    ^~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/talloc.h:345:143: note: expanded from:
  ...__location__); __talloc_steal_ret; })
                    ^~~~~~~~~~~~~~~~~~
1 warning generated.

CXX -O2 lib/message.o
lib/message.cc:791:5: warning: expression result unused [-Wunused-value]
    talloc_reference (message, message->tag_list);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/talloc.h:932:36: note: expanded from:
  ...(_TALLOC_TYPEOF(ptr))_talloc_reference_loc((ctx),(ptr), __location__)
     ^                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

Check talloc_reference() return value, and explicitly ignore
talloc_steal() return value as it has no failure modes, to silence the
warnings.
---
 lib/message.cc |    4 +++-
 lib/tags.c     |    2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 978de06..320901f 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -788,7 +788,9 @@ notmuch_message_get_tags (notmuch_message_t *message)
      * possible to modify the message tags (which talloc_unlink's the
      * current list from the message) while still iterating because
      * the iterator will keep the current list alive. */
-    talloc_reference (message, message->tag_list);
+    if (!talloc_reference (message, message->tag_list))
+	return NULL;
+
     return tags;
 }
 
diff --git a/lib/tags.c b/lib/tags.c
index c58924f..b7e5602 100644
--- a/lib/tags.c
+++ b/lib/tags.c
@@ -40,7 +40,7 @@ _notmuch_tags_create (const void *ctx, notmuch_string_list_t *list)
 	return NULL;
 
     tags->iterator = list->head;
-    talloc_steal (tags, list);
+    (void) talloc_steal (tags, list);
 
     return tags;
 }
-- 
1.7.2.5

Re: [PATCH] lib: fix warnings when building with clang

[Ethan Glasser-Camp]

Jani Nikula <jani@nikula.org> writes:

> Building notmuch with CC=clang and CXX=clang++ produces the warnings:
>
> CC -O2 lib/tags.o
> lib/tags.c:43:5: warning: expression result unused [-Wunused-value]
>     talloc_steal (tags, list);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~
> /usr/include/talloc.h:345:143: note: expanded from:
>   ...__location__); __talloc_steal_ret; })
>                     ^~~~~~~~~~~~~~~~~~
> 1 warning generated.
>
> CXX -O2 lib/message.o
> lib/message.cc:791:5: warning: expression result unused [-Wunused-value]
>     talloc_reference (message, message->tag_list);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /usr/include/talloc.h:932:36: note: expanded from:
>   ...(_TALLOC_TYPEOF(ptr))_talloc_reference_loc((ctx),(ptr), __location__)
>      ^                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 warning generated.
>
> Check talloc_reference() return value, and explicitly ignore
> talloc_steal() return value as it has no failure modes, to silence the
> warnings.
> ---
>  lib/message.cc |    4 +++-
>  lib/tags.c     |    2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/message.cc b/lib/message.cc
> index 978de06..320901f 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -788,7 +788,9 @@ notmuch_message_get_tags (notmuch_message_t *message)
>       * possible to modify the message tags (which talloc_unlink's the
>       * current list from the message) while still iterating because
>       * the iterator will keep the current list alive. */
> -    talloc_reference (message, message->tag_list);
> +    if (!talloc_reference (message, message->tag_list))
> +	return NULL;
> +
>      return tags;
>  }

Hi! What you did with talloc_steal is obviously fine. 

I'd be happier about what you did with talloc_reference() if there were
precedent, or a clearly-articulated convention for notmuch. Instead this
is the third use in the codebase that I can see, and the other two are
each unique to themselves. In mime-node.c we print an "out-of-memory"
error and in lib/filenames.c we cast (void) talloc_reference (...), I
guess figuring that we're pretty much hosed anyhow if we run out of
memory.

Why return NULL here? It seems like if talloc_reference fails, we're
going to crash eventually, so we should print an error to explain our
impending doom. I'd guess you're uneasy printing anything from lib/, but
still want to signal an error, and the only way you can do so is to
return NULL. I guess that silences the compiler warning, but it's not
really the correct way to handle the error IMO. On the other hand, it's
such a weird corner case that I don't even think it merits a FIXME
comment.

How about an assert instead of a return NULL?

Ethan

Re: [PATCH] lib: fix warnings when building with clang

[Jani Nikula]

On Sun, 21 Oct 2012, Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> wrote:
> Jani Nikula <jani@nikula.org> writes:
>
>> Building notmuch with CC=clang and CXX=clang++ produces the warnings:
>>
>> CC -O2 lib/tags.o
>> lib/tags.c:43:5: warning: expression result unused [-Wunused-value]
>>     talloc_steal (tags, list);
>>     ^~~~~~~~~~~~~~~~~~~~~~~~~
>> /usr/include/talloc.h:345:143: note: expanded from:
>>   ...__location__); __talloc_steal_ret; })
>>                     ^~~~~~~~~~~~~~~~~~
>> 1 warning generated.
>>
>> CXX -O2 lib/message.o
>> lib/message.cc:791:5: warning: expression result unused [-Wunused-value]
>>     talloc_reference (message, message->tag_list);
>>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> /usr/include/talloc.h:932:36: note: expanded from:
>>   ...(_TALLOC_TYPEOF(ptr))_talloc_reference_loc((ctx),(ptr), __location__)
>>      ^                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 1 warning generated.
>>
>> Check talloc_reference() return value, and explicitly ignore
>> talloc_steal() return value as it has no failure modes, to silence the
>> warnings.
>> ---
>>  lib/message.cc |    4 +++-
>>  lib/tags.c     |    2 +-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/message.cc b/lib/message.cc
>> index 978de06..320901f 100644
>> --- a/lib/message.cc
>> +++ b/lib/message.cc
>> @@ -788,7 +788,9 @@ notmuch_message_get_tags (notmuch_message_t *message)
>>       * possible to modify the message tags (which talloc_unlink's the
>>       * current list from the message) while still iterating because
>>       * the iterator will keep the current list alive. */
>> -    talloc_reference (message, message->tag_list);
>> +    if (!talloc_reference (message, message->tag_list))
>> +	return NULL;
>> +
>>      return tags;
>>  }
>
> Hi! What you did with talloc_steal is obviously fine. 
>
> I'd be happier about what you did with talloc_reference() if there were
> precedent, or a clearly-articulated convention for notmuch. Instead this
> is the third use in the codebase that I can see, and the other two are
> each unique to themselves. In mime-node.c we print an "out-of-memory"
> error and in lib/filenames.c we cast (void) talloc_reference (...), I
> guess figuring that we're pretty much hosed anyhow if we run out of
> memory.
>
> Why return NULL here? It seems like if talloc_reference fails, we're
> going to crash eventually, so we should print an error to explain our
> impending doom. I'd guess you're uneasy printing anything from lib/, but
> still want to signal an error, and the only way you can do so is to
> return NULL. I guess that silences the compiler warning, but it's not
> really the correct way to handle the error IMO. On the other hand, it's
> such a weird corner case that I don't even think it merits a FIXME
> comment.
>
> How about an assert instead of a return NULL?

No. I don't think a library should assert, exit, or print to stderr on
this sort of thing. It's up to the calling application. Even if it
probably doesn't have many choices left, given how much memory
talloc_reference needs (not much).

Ignoring the talloc_reference return value with (void) is just wrong,
and the caller of notmuch_message_get_tags should anticipate a NULL
return. So IMHO that's the pragmatic thing to do in this mostly
theoretical situation, the biggest change being silencing the warning.


BR,
Jani.

Re: [PATCH] lib: fix warnings when building with clang

[Tomi Ollila]

On Sun, Oct 21 2012, Jani Nikula <jani@nikula.org> wrote:

> On Sun, 21 Oct 2012, Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> wrote:
>> Jani Nikula <jani@nikula.org> writes:
>>
>>> Building notmuch with CC=clang and CXX=clang++ produces the warnings:
>>>
>>> CC -O2 lib/tags.o
>>> lib/tags.c:43:5: warning: expression result unused [-Wunused-value]
>>>     talloc_steal (tags, list);
>>>     ^~~~~~~~~~~~~~~~~~~~~~~~~
>>> /usr/include/talloc.h:345:143: note: expanded from:
>>>   ...__location__); __talloc_steal_ret; })
>>>                     ^~~~~~~~~~~~~~~~~~
>>> 1 warning generated.
>>>
>>> CXX -O2 lib/message.o
>>> lib/message.cc:791:5: warning: expression result unused [-Wunused-value]
>>>     talloc_reference (message, message->tag_list);
>>>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> /usr/include/talloc.h:932:36: note: expanded from:
>>>   ...(_TALLOC_TYPEOF(ptr))_talloc_reference_loc((ctx),(ptr), __location__)
>>>      ^                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> 1 warning generated.
>>>
>>> Check talloc_reference() return value, and explicitly ignore
>>> talloc_steal() return value as it has no failure modes, to silence the
>>> warnings.
>>> ---
>>>  lib/message.cc |    4 +++-
>>>  lib/tags.c     |    2 +-
>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/message.cc b/lib/message.cc
>>> index 978de06..320901f 100644
>>> --- a/lib/message.cc
>>> +++ b/lib/message.cc
>>> @@ -788,7 +788,9 @@ notmuch_message_get_tags (notmuch_message_t *message)
>>>       * possible to modify the message tags (which talloc_unlink's the
>>>       * current list from the message) while still iterating because
>>>       * the iterator will keep the current list alive. */
>>> -    talloc_reference (message, message->tag_list);
>>> +    if (!talloc_reference (message, message->tag_list))
>>> +	return NULL;
>>> +
>>>      return tags;
>>>  }
>>
>> Hi! What you did with talloc_steal is obviously fine. 
>>
>> I'd be happier about what you did with talloc_reference() if there were
>> precedent, or a clearly-articulated convention for notmuch. Instead this
>> is the third use in the codebase that I can see, and the other two are
>> each unique to themselves. In mime-node.c we print an "out-of-memory"
>> error and in lib/filenames.c we cast (void) talloc_reference (...), I
>> guess figuring that we're pretty much hosed anyhow if we run out of
>> memory.
>>
>> Why return NULL here? It seems like if talloc_reference fails, we're
>> going to crash eventually, so we should print an error to explain our
>> impending doom. I'd guess you're uneasy printing anything from lib/, but
>> still want to signal an error, and the only way you can do so is to
>> return NULL. I guess that silences the compiler warning, but it's not
>> really the correct way to handle the error IMO. On the other hand, it's
>> such a weird corner case that I don't even think it merits a FIXME
>> comment.
>>
>> How about an assert instead of a return NULL?
>
> No. I don't think a library should assert, exit, or print to stderr on
> this sort of thing. It's up to the calling application. Even if it
> probably doesn't have many choices left, given how much memory
> talloc_reference needs (not much).
>
> Ignoring the talloc_reference return value with (void) is just wrong,
> and the caller of notmuch_message_get_tags should anticipate a NULL
> return. So IMHO that's the pragmatic thing to do in this mostly
> theoretical situation, the biggest change being silencing the warning.

I agree that the best library can do is to return NULL (if talloc had
a place in ctx to store error indication that could be used but I did
not see any in quick look -- and using global there is not a good idea)

but, before returning NULL should 'tags' be freed.

Additionally, should lib/filenames.c be changed to have code:

if (unlikely (talloc_reference(filenames, list) == NULL)) {
    talloc_free (filenames);
    return NULL;
}

> BR,
> Jani.

(btw, what are the chances that program crashes before returning NULL
due to page fault in stack frame allocation ???)

Tomi

Re: [PATCH] lib: fix warnings when building with clang

[Tomi Ollila]

On Mon, Oct 22 2012, Tomi Ollila wrote:

> On Sun, Oct 21 2012, Jani Nikula <jani@nikula.org> wrote:
>
>> On Sun, 21 Oct 2012, Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> wrote:
>>> Jani Nikula <jani@nikula.org> writes:
>>>
>>>> Building notmuch with CC=clang and CXX=clang++ produces the warnings:
>>>>
>>>> CC -O2 lib/tags.o
>>>> lib/tags.c:43:5: warning: expression result unused [-Wunused-value]
>>>>     talloc_steal (tags, list);
>>>>     ^~~~~~~~~~~~~~~~~~~~~~~~~
>>>> /usr/include/talloc.h:345:143: note: expanded from:
>>>>   ...__location__); __talloc_steal_ret; })
>>>>                     ^~~~~~~~~~~~~~~~~~
>>>> 1 warning generated.
>>>>
>>>> CXX -O2 lib/message.o
>>>> lib/message.cc:791:5: warning: expression result unused [-Wunused-value]
>>>>     talloc_reference (message, message->tag_list);
>>>>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> /usr/include/talloc.h:932:36: note: expanded from:
>>>>   ...(_TALLOC_TYPEOF(ptr))_talloc_reference_loc((ctx),(ptr), __location__)
>>>>      ^                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> 1 warning generated.
>>>>
>>>> Check talloc_reference() return value, and explicitly ignore
>>>> talloc_steal() return value as it has no failure modes, to silence the
>>>> warnings.
>>>> ---
>>>>  lib/message.cc |    4 +++-
>>>>  lib/tags.c     |    2 +-
>>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/message.cc b/lib/message.cc
>>>> index 978de06..320901f 100644
>>>> --- a/lib/message.cc
>>>> +++ b/lib/message.cc
>>>> @@ -788,7 +788,9 @@ notmuch_message_get_tags (notmuch_message_t *message)
>>>>       * possible to modify the message tags (which talloc_unlink's the
>>>>       * current list from the message) while still iterating because
>>>>       * the iterator will keep the current list alive. */
>>>> -    talloc_reference (message, message->tag_list);
>>>> +    if (!talloc_reference (message, message->tag_list))
>>>> +	return NULL;
>>>> +
>>>>      return tags;
>>>>  }
>>>
>>> Hi! What you did with talloc_steal is obviously fine. 
>>>
>>> I'd be happier about what you did with talloc_reference() if there were
>>> precedent, or a clearly-articulated convention for notmuch. Instead this
>>> is the third use in the codebase that I can see, and the other two are
>>> each unique to themselves. In mime-node.c we print an "out-of-memory"
>>> error and in lib/filenames.c we cast (void) talloc_reference (...), I
>>> guess figuring that we're pretty much hosed anyhow if we run out of
>>> memory.
>>>
>>> Why return NULL here? It seems like if talloc_reference fails, we're
>>> going to crash eventually, so we should print an error to explain our
>>> impending doom. I'd guess you're uneasy printing anything from lib/, but
>>> still want to signal an error, and the only way you can do so is to
>>> return NULL. I guess that silences the compiler warning, but it's not
>>> really the correct way to handle the error IMO. On the other hand, it's
>>> such a weird corner case that I don't even think it merits a FIXME
>>> comment.
>>>
>>> How about an assert instead of a return NULL?
>>
>> No. I don't think a library should assert, exit, or print to stderr on
>> this sort of thing. It's up to the calling application. Even if it
>> probably doesn't have many choices left, given how much memory
>> talloc_reference needs (not much).
>>
>> Ignoring the talloc_reference return value with (void) is just wrong,
>> and the caller of notmuch_message_get_tags should anticipate a NULL
>> return. So IMHO that's the pragmatic thing to do in this mostly
>> theoretical situation, the biggest change being silencing the warning.
>
> I agree that the best library can do is to return NULL (if talloc had
> a place in ctx to store error indication that could be used but I did
> not see any in quick look -- and using global there is not a good idea)
>
> but, before returning NULL should 'tags' be freed.

Ah, this is talloc() stuff, so the patch is good in this part.

> Additionally, should lib/filenames.c be changed to have code:
>
> if (unlikely (talloc_reference(filenames, list) == NULL)) {
>     talloc_free (filenames);
>     return NULL;
> }

More like 

if (unlikely (talloc_reference(filenames, list) == NULL))
    return NULL;

But that is out of scope of this patch...


so, +1

Tomi

>> BR,
>> Jani.
>
> (btw, what are the chances that program crashes before returning NULL
> due to page fault in stack frame allocation ???)
>
> Tomi

Re: [PATCH] lib: fix warnings when building with clang

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Check talloc_reference() return value, and explicitly ignore
> talloc_steal() return value as it has no failure modes, to silence the
> warnings.

pushed,

d