<div class="gmail_quote">On 24 March 2011 18:40, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

<div class="gmail_quote"><div class="im">On Thu, Mar 24, 2011 at 6:32 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


We implement __attribute__((nonnull)), but only test for it on CallExpr nodes. When calling a constructor, the AST only has a CXXConstructorExpr with no CallExpr, so the warning doesn't fire. This patch adds the same test that CallExpr uses to CXXConstructorExpr.<div>




<br></div><div>Please review!</div></blockquote><div><br></div></div><div>LGTM</div><div><br></div><div>I find a few things about the existing code strange, but your patch doesn't introduce any of them.</div><div><br>

</div>
<div>Why do we loop over attributes inside of CheckNonNull... as well as in the callers?</div></div></blockquote><div><br></div><div>Because this is legal:</div><div><br></div><div>  void test(void *, void *, void *) __attribute__((nonnull(1, 2))) __attribute__((nonnull(3)));</div>

<div><br></div><div>The inner loop finds the "1, 2" inside, the outer loop finds the nonnull attribute(s) amongst all the attributes.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

<div class="gmail_quote"><div> I understand why the caller loop is necessary, I don't understand why we still need the inner loop, or why we don't sink the callers' loops into CheckNonNull, but maybe other code needs this.</div>


<div><br></div><div>Also, It seems like the location of the argument which is erroneously NULL would be a better location than the location of the call...</div></div>
</blockquote></div><br><div>We highlight the expression's source range. It looks like this:</div><div><br></div><div><div><font class="Apple-style-span" face="'courier new', monospace">foo.cc:10:3: warning: null passed to a callee which requires a non-null argument</font></div>

<div><font class="Apple-style-span" face="'courier new', monospace">      [-Wnonnull]</font></div><div><font class="Apple-style-span" face="'courier new', monospace">  Test(NULL);</font></div><div><font class="Apple-style-span" face="'courier new', monospace">  ^    ~~~~</font></div>

<div><font class="Apple-style-span" face="'courier new', monospace">foo.cc:11:8: warning: null passed to a callee which requires a non-null argument</font></div><div><font class="Apple-style-span" face="'courier new', monospace">      [-Wnonnull]</font></div>

<div><font class="Apple-style-span" face="'courier new', monospace">  Test t(NULL);</font></div><div><font class="Apple-style-span" face="'courier new', monospace">       ^ ~~~~</font></div><div><font class="Apple-style-span" face="'courier new', monospace">foo.cc:12:3: warning: null passed to a callee which requires a non-null argument</font></div>

<div><font class="Apple-style-span" face="'courier new', monospace">      [-Wnonnull]</font></div><div><font class="Apple-style-span" face="'courier new', monospace">  test2(0);</font></div><div><font class="Apple-style-span" face="'courier new', monospace">  ^     ~</font></div>

</div><div><br></div><div>Nick</div>