From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 67391431FAF for ; Sun, 21 Oct 2012 11:44:25 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.7 X-Spam-Level: X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Ie8enu8O8nhj for ; Sun, 21 Oct 2012 11:44:24 -0700 (PDT) Received: from mail-la0-f53.google.com (mail-la0-f53.google.com [209.85.215.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id 7FA81431FAE for ; Sun, 21 Oct 2012 11:44:24 -0700 (PDT) Received: by mail-la0-f53.google.com with SMTP id l5so1405262lah.26 for ; Sun, 21 Oct 2012 11:44:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:to:subject:in-reply-to:references:user-agent:date:message-id :mime-version:content-type:x-gm-message-state; bh=LwZFQlgY5/Io+ENbkeV45QkSyOZiuV95gmvU4ddhMZA=; b=hdxbjHFWAEQZbbYFNjrdJ0Sopp4g60eVx+ID8NrL5MIKmy4cNWUW1nCQAJS2FTmOpn 6lpS3jIcE8wPvsvGNBSmjlED36ZFq5BhOednrT8W8H+ngnhYejRLrGChFXzOrcl4ToTe /c03CDPk6ZZ51Sdat29qBo8iBgDt7XpSdfMPUXK+ij/8RviFZ4NR6HalMdD43GQrqXOH cyLZ7ZNUe8e4QYHCuameSCzcsEuOcGu6IjgtIL2gCqj6qfJx0dsQ7c7O7bcf661GJmJc W6HNld1MIoUsfmzjwwHBNJsYbkXdfR9P9xKZCo5aiayWicRjyYLtYmZCOGiKY9kvXqZn 4npA== Received: by 10.112.39.233 with SMTP id s9mr2838268lbk.71.1350845061406; Sun, 21 Oct 2012 11:44:21 -0700 (PDT) Received: from localhost (dsl-hkibrasgw4-fe51df00-27.dhcp.inet.fi. [80.223.81.27]) by mx.google.com with ESMTPS id z9sm2393891lbk.15.2012.10.21.11.44.18 (version=SSLv3 cipher=OTHER); Sun, 21 Oct 2012 11:44:20 -0700 (PDT) From: Jani Nikula To: Ethan Glasser-Camp , 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 X-Gm-Message-State: ALoCoQnf8hYYFAHRfPRTnjuR2LaJ6bOxw6qoCRdBE4nH7VJsqNj8H4i3RtETqFKywB1uvHMLIEBH X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 21 Oct 2012 18:44:25 -0000 On Sun, 21 Oct 2012, Ethan Glasser-Camp wrote: > Jani Nikula 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.