From: Jani Nikula Date: Sun, 21 Oct 2012 18:44:17 +0000 (+0300) Subject: Re: [PATCH] lib: fix warnings when building with clang X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=d812d6d35882c15fbc72c64c65485d0fa65c95c9;p=notmuch-archives.git Re: [PATCH] lib: fix warnings when building with clang --- diff --git a/52/e508a4c7c866e13907ffe956e7791051849fcd b/52/e508a4c7c866e13907ffe956e7791051849fcd new file mode 100644 index 000000000..26a14d984 --- /dev/null +++ b/52/e508a4c7c866e13907ffe956e7791051849fcd @@ -0,0 +1,148 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +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.