From: Jani Nikula <jani@nikula.org>
To: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH] lib: fix warnings when building with clang
In-Reply-To: <87pq4c61hc.fsf@betacantrips.com>
References: <1349076971-2065-1-git-send-email-jani@nikula.org>
	<87pq4c61hc.fsf@betacantrips.com>
User-Agent: Notmuch/0.14+46~g272a1f1 (http://notmuchmail.org) Emacs/23.4.1
	(i686-pc-linux-gnu)
Date: Sun, 21 Oct 2012 21:44:17 +0300
Message-ID: <87vce3vf5q.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

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.