52/e508a4c7c866e13907ffe956e7791051849fcd

   1 Return-Path: <jani@nikula.org>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 67391431FAF\r
   6         for <notmuch@notmuchmail.org>; Sun, 21 Oct 2012 11:44:25 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id Ie8enu8O8nhj for <notmuch@notmuchmail.org>;\r
  16         Sun, 21 Oct 2012 11:44:24 -0700 (PDT)\r
  17 Received: from mail-la0-f53.google.com (mail-la0-f53.google.com\r
  18         [209.85.215.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  19         (No client certificate requested)\r
  20         by olra.theworths.org (Postfix) with ESMTPS id 7FA81431FAE\r
  21         for <notmuch@notmuchmail.org>; Sun, 21 Oct 2012 11:44:24 -0700 (PDT)\r
  22 Received: by mail-la0-f53.google.com with SMTP id l5so1405262lah.26\r
  23         for <notmuch@notmuchmail.org>; Sun, 21 Oct 2012 11:44:21 -0700 (PDT)\r
  24 X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;\r
  25         d=google.com; s=20120113;\r
  26         h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
  27         :mime-version:content-type:x-gm-message-state;\r
  28         bh=LwZFQlgY5/Io+ENbkeV45QkSyOZiuV95gmvU4ddhMZA=;\r
  29         b=hdxbjHFWAEQZbbYFNjrdJ0Sopp4g60eVx+ID8NrL5MIKmy4cNWUW1nCQAJS2FTmOpn\r
  30         6lpS3jIcE8wPvsvGNBSmjlED36ZFq5BhOednrT8W8H+ngnhYejRLrGChFXzOrcl4ToTe\r
  31         /c03CDPk6ZZ51Sdat29qBo8iBgDt7XpSdfMPUXK+ij/8RviFZ4NR6HalMdD43GQrqXOH\r
  32         cyLZ7ZNUe8e4QYHCuameSCzcsEuOcGu6IjgtIL2gCqj6qfJx0dsQ7c7O7bcf661GJmJc\r
  33         W6HNld1MIoUsfmzjwwHBNJsYbkXdfR9P9xKZCo5aiayWicRjyYLtYmZCOGiKY9kvXqZn\r
  34         4npA==\r
  35 Received: by 10.112.39.233 with SMTP id s9mr2838268lbk.71.1350845061406;\r
  36         Sun, 21 Oct 2012 11:44:21 -0700 (PDT)\r
  37 Received: from localhost (dsl-hkibrasgw4-fe51df00-27.dhcp.inet.fi.\r
  38         [80.223.81.27])\r
  39         by mx.google.com with ESMTPS id z9sm2393891lbk.15.2012.10.21.11.44.18\r
  40         (version=SSLv3 cipher=OTHER); Sun, 21 Oct 2012 11:44:20 -0700 (PDT)\r
  41 From: Jani Nikula <jani@nikula.org>\r
  42 To: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>, notmuch@notmuchmail.org\r
  43 Subject: Re: [PATCH] lib: fix warnings when building with clang\r
  44 In-Reply-To: <87pq4c61hc.fsf@betacantrips.com>\r
  45 References: <1349076971-2065-1-git-send-email-jani@nikula.org>\r
  46         <87pq4c61hc.fsf@betacantrips.com>\r
  47 User-Agent: Notmuch/0.14+46~g272a1f1 (http://notmuchmail.org) Emacs/23.4.1\r
  48         (i686-pc-linux-gnu)\r
  49 Date: Sun, 21 Oct 2012 21:44:17 +0300\r
  50 Message-ID: <87vce3vf5q.fsf@nikula.org>\r
  51 MIME-Version: 1.0\r
  52 Content-Type: text/plain; charset=us-ascii\r
  53 X-Gm-Message-State:\r
  54  ALoCoQnf8hYYFAHRfPRTnjuR2LaJ6bOxw6qoCRdBE4nH7VJsqNj8H4i3RtETqFKywB1uvHMLIEBH\r
  55 X-BeenThere: notmuch@notmuchmail.org\r
  56 X-Mailman-Version: 2.1.13\r
  57 Precedence: list\r
  58 List-Id: "Use and development of the notmuch mail system."\r
  59         <notmuch.notmuchmail.org>\r
  60 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  61         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  62 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  63 List-Post: <mailto:notmuch@notmuchmail.org>\r
  64 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  65 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  66         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  67 X-List-Received-Date: Sun, 21 Oct 2012 18:44:25 -0000\r
  68 \r
  69 On Sun, 21 Oct 2012, Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> wrote:\r
  70 > Jani Nikula <jani@nikula.org> writes:\r
  71 >\r
  72 >> Building notmuch with CC=clang and CXX=clang++ produces the warnings:\r
  73 >>\r
  74 >> CC -O2 lib/tags.o\r
  75 >> lib/tags.c:43:5: warning: expression result unused [-Wunused-value]\r
  76 >>     talloc_steal (tags, list);\r
  77 >>     ^~~~~~~~~~~~~~~~~~~~~~~~~\r
  78 >> /usr/include/talloc.h:345:143: note: expanded from:\r
  79 >>   ...__location__); __talloc_steal_ret; })\r
  80 >>                     ^~~~~~~~~~~~~~~~~~\r
  81 >> 1 warning generated.\r
  82 >>\r
  83 >> CXX -O2 lib/message.o\r
  84 >> lib/message.cc:791:5: warning: expression result unused [-Wunused-value]\r
  85 >>     talloc_reference (message, message->tag_list);\r
  86 >>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\r
  87 >> /usr/include/talloc.h:932:36: note: expanded from:\r
  88 >>   ...(_TALLOC_TYPEOF(ptr))_talloc_reference_loc((ctx),(ptr), __location__)\r
  89 >>      ^                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\r
  90 >> 1 warning generated.\r
  91 >>\r
  92 >> Check talloc_reference() return value, and explicitly ignore\r
  93 >> talloc_steal() return value as it has no failure modes, to silence the\r
  94 >> warnings.\r
  95 >> ---\r
  96 >>  lib/message.cc |    4 +++-\r
  97 >>  lib/tags.c     |    2 +-\r
  98 >>  2 files changed, 4 insertions(+), 2 deletions(-)\r
  99 >>\r
 100 >> diff --git a/lib/message.cc b/lib/message.cc\r
 101 >> index 978de06..320901f 100644\r
 102 >> --- a/lib/message.cc\r
 103 >> +++ b/lib/message.cc\r
 104 >> @@ -788,7 +788,9 @@ notmuch_message_get_tags (notmuch_message_t *message)\r
 105 >>       * possible to modify the message tags (which talloc_unlink's the\r
 106 >>       * current list from the message) while still iterating because\r
 107 >>       * the iterator will keep the current list alive. */\r
 108 >> -    talloc_reference (message, message->tag_list);\r
 109 >> +    if (!talloc_reference (message, message->tag_list))\r
 110 >> +    return NULL;\r
 111 >> +\r
 112 >>      return tags;\r
 113 >>  }\r
 114 >\r
 115 > Hi! What you did with talloc_steal is obviously fine. \r
 116 >\r
 117 > I'd be happier about what you did with talloc_reference() if there were\r
 118 > precedent, or a clearly-articulated convention for notmuch. Instead this\r
 119 > is the third use in the codebase that I can see, and the other two are\r
 120 > each unique to themselves. In mime-node.c we print an "out-of-memory"\r
 121 > error and in lib/filenames.c we cast (void) talloc_reference (...), I\r
 122 > guess figuring that we're pretty much hosed anyhow if we run out of\r
 123 > memory.\r
 124 >\r
 125 > Why return NULL here? It seems like if talloc_reference fails, we're\r
 126 > going to crash eventually, so we should print an error to explain our\r
 127 > impending doom. I'd guess you're uneasy printing anything from lib/, but\r
 128 > still want to signal an error, and the only way you can do so is to\r
 129 > return NULL. I guess that silences the compiler warning, but it's not\r
 130 > really the correct way to handle the error IMO. On the other hand, it's\r
 131 > such a weird corner case that I don't even think it merits a FIXME\r
 132 > comment.\r
 133 >\r
 134 > How about an assert instead of a return NULL?\r
 135 \r
 136 No. I don't think a library should assert, exit, or print to stderr on\r
 137 this sort of thing. It's up to the calling application. Even if it\r
 138 probably doesn't have many choices left, given how much memory\r
 139 talloc_reference needs (not much).\r
 140 \r
 141 Ignoring the talloc_reference return value with (void) is just wrong,\r
 142 and the caller of notmuch_message_get_tags should anticipate a NULL\r
 143 return. So IMHO that's the pragmatic thing to do in this mostly\r
 144 theoretical situation, the biggest change being silencing the warning.\r
 145 \r
 146 \r
 147 BR,\r
 148 Jani.\r