66/94d36309a79bfce9bb86802d5e28bbcc487c8d

   1 Return-Path: <ethan.glasser.camp@gmail.com>\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 543AE431FAF\r
   6         for <notmuch@notmuchmail.org>; Sat, 20 Oct 2012 18:46:45 -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.799\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
  12         tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
  13         FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  14 Received: from olra.theworths.org ([127.0.0.1])\r
  15         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  16         with ESMTP id XDU51ezOWXJZ for <notmuch@notmuchmail.org>;\r
  17         Sat, 20 Oct 2012 18:46:44 -0700 (PDT)\r
  18 Received: from mail-vc0-f181.google.com (mail-vc0-f181.google.com\r
  19         [209.85.220.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id 9405C431FAE\r
  22         for <notmuch@notmuchmail.org>; Sat, 20 Oct 2012 18:46:44 -0700 (PDT)\r
  23 Received: by mail-vc0-f181.google.com with SMTP id n11so1909718vch.26\r
  24         for <notmuch@notmuchmail.org>; Sat, 20 Oct 2012 18:46:42 -0700 (PDT)\r
  25 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113;\r
  26         h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
  27         :mime-version:content-type;\r
  28         bh=im1CbZ8uFOgVenJY3mO7AtoSAq28Pgp+cjOV+l2zAhQ=;\r
  29         b=nVNIiHE8ZLfgVINsPs1Ggnpjv4llZQv/2HLzjbZ1ziUnlolEt4ZCHHbgjtMY+Y0aFE\r
  30         0inYfK0YITIaUnteBL7iUmwxeDnx+g+p48BFzXliG78+ZDbl29oSNv8k/eRLMppPIuLH\r
  31         YMUdo3j8eXdSpWKf2P9xHWSWQ4sF1FPpiN9vXx54pcJbdbvAvFuv25LyTmfD7B+IOwY5\r
  32         2ui8wRysOVk9jME+DZ7/TFtJ8sBlvCtw2HgAK/LIOaF4ilIlSMHQQNwM5Ru8Iif79teD\r
  33         Ra2C/0U+4e85eArjNcepwMiBiMTVcjgbqlX8vXG591qiUcIZOU4g6syLHpX4/CfgWvm3\r
  34         NWrg==\r
  35 Received: by 10.220.220.5 with SMTP id hw5mr8024596vcb.53.1350784002732;\r
  36         Sat, 20 Oct 2012 18:46:42 -0700 (PDT)\r
  37 Received: from smtp.gmail.com (p70-80.acedsl.com. [66.114.70.80])\r
  38         by mx.google.com with ESMTPS id y15sm6004289vdt.9.2012.10.20.18.46.40\r
  39         (version=TLSv1/SSLv3 cipher=OTHER);\r
  40         Sat, 20 Oct 2012 18:46:41 -0700 (PDT)\r
  41 From: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>\r
  42 To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org\r
  43 Subject: Re: [PATCH] lib: fix warnings when building with clang\r
  44 In-Reply-To: <1349076971-2065-1-git-send-email-jani@nikula.org>\r
  45 References: <1349076971-2065-1-git-send-email-jani@nikula.org>\r
  46 User-Agent: Notmuch/0.14+45~g6ea9330 (http://notmuchmail.org) Emacs/23.3.1\r
  47         (x86_64-pc-linux-gnu)\r
  48 Date: Sat, 20 Oct 2012 21:46:39 -0400\r
  49 Message-ID: <87pq4c61hc.fsf@betacantrips.com>\r
  50 MIME-Version: 1.0\r
  51 Content-Type: text/plain; charset=us-ascii\r
  52 X-BeenThere: notmuch@notmuchmail.org\r
  53 X-Mailman-Version: 2.1.13\r
  54 Precedence: list\r
  55 List-Id: "Use and development of the notmuch mail system."\r
  56         <notmuch.notmuchmail.org>\r
  57 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  58         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  59 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  60 List-Post: <mailto:notmuch@notmuchmail.org>\r
  61 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  62 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  63         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  64 X-List-Received-Date: Sun, 21 Oct 2012 01:46:45 -0000\r
  65 \r
  66 Jani Nikula <jani@nikula.org> writes:\r
  67 \r
  68 > Building notmuch with CC=clang and CXX=clang++ produces the warnings:\r
  69 >\r
  70 > CC -O2 lib/tags.o\r
  71 > lib/tags.c:43:5: warning: expression result unused [-Wunused-value]\r
  72 >     talloc_steal (tags, list);\r
  73 >     ^~~~~~~~~~~~~~~~~~~~~~~~~\r
  74 > /usr/include/talloc.h:345:143: note: expanded from:\r
  75 >   ...__location__); __talloc_steal_ret; })\r
  76 >                     ^~~~~~~~~~~~~~~~~~\r
  77 > 1 warning generated.\r
  78 >\r
  79 > CXX -O2 lib/message.o\r
  80 > lib/message.cc:791:5: warning: expression result unused [-Wunused-value]\r
  81 >     talloc_reference (message, message->tag_list);\r
  82 >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\r
  83 > /usr/include/talloc.h:932:36: note: expanded from:\r
  84 >   ...(_TALLOC_TYPEOF(ptr))_talloc_reference_loc((ctx),(ptr), __location__)\r
  85 >      ^                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\r
  86 > 1 warning generated.\r
  87 >\r
  88 > Check talloc_reference() return value, and explicitly ignore\r
  89 > talloc_steal() return value as it has no failure modes, to silence the\r
  90 > warnings.\r
  91 > ---\r
  92 >  lib/message.cc |    4 +++-\r
  93 >  lib/tags.c     |    2 +-\r
  94 >  2 files changed, 4 insertions(+), 2 deletions(-)\r
  95 >\r
  96 > diff --git a/lib/message.cc b/lib/message.cc\r
  97 > index 978de06..320901f 100644\r
  98 > --- a/lib/message.cc\r
  99 > +++ b/lib/message.cc\r
 100 > @@ -788,7 +788,9 @@ notmuch_message_get_tags (notmuch_message_t *message)\r
 101 >       * possible to modify the message tags (which talloc_unlink's the\r
 102 >       * current list from the message) while still iterating because\r
 103 >       * the iterator will keep the current list alive. */\r
 104 > -    talloc_reference (message, message->tag_list);\r
 105 > +    if (!talloc_reference (message, message->tag_list))\r
 106 > +     return NULL;\r
 107 > +\r
 108 >      return tags;\r
 109 >  }\r
 110 \r
 111 Hi! What you did with talloc_steal is obviously fine. \r
 112 \r
 113 I'd be happier about what you did with talloc_reference() if there were\r
 114 precedent, or a clearly-articulated convention for notmuch. Instead this\r
 115 is the third use in the codebase that I can see, and the other two are\r
 116 each unique to themselves. In mime-node.c we print an "out-of-memory"\r
 117 error and in lib/filenames.c we cast (void) talloc_reference (...), I\r
 118 guess figuring that we're pretty much hosed anyhow if we run out of\r
 119 memory.\r
 120 \r
 121 Why return NULL here? It seems like if talloc_reference fails, we're\r
 122 going to crash eventually, so we should print an error to explain our\r
 123 impending doom. I'd guess you're uneasy printing anything from lib/, but\r
 124 still want to signal an error, and the only way you can do so is to\r
 125 return NULL. I guess that silences the compiler warning, but it's not\r
 126 really the correct way to handle the error IMO. On the other hand, it's\r
 127 such a weird corner case that I don't even think it merits a FIXME\r
 128 comment.\r
 129 \r
 130 How about an assert instead of a return NULL?\r
 131 \r
 132 Ethan\r