[cfe-commits] fix attribute nonnull to apply to constructors

[Nick Lewycky]

On 24 March 2011 18:40, Chandler Carruth <chandlerc at google.com> wrote:

> On Thu, Mar 24, 2011 at 6:32 PM, Nick Lewycky <nlewycky at google.com> wrote:
>
>> 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.
>>
>> Please review!
>>
>
> LGTM
>
> I find a few things about the existing code strange, but your patch doesn't
> introduce any of them.
>
>  Why do we loop over attributes inside of CheckNonNull... as well as in
> the callers?
>

Because this is legal:

  void test(void *, void *, void *) __attribute__((nonnull(1, 2)))
__attribute__((nonnull(3)));

The inner loop finds the "1, 2" inside, the outer loop finds the nonnull
attribute(s) amongst all the attributes.


>  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.
>
> Also, It seems like the location of the argument which is erroneously NULL
> would be a better location than the location of the call...
>

We highlight the expression's source range. It looks like this:

foo.cc:10:3: warning: null passed to a callee which requires a non-null
argument
      [-Wnonnull]
  Test(NULL);
  ^    ~~~~
foo.cc:11:8: warning: null passed to a callee which requires a non-null
argument
      [-Wnonnull]
  Test t(NULL);
       ^ ~~~~
foo.cc:12:3: warning: null passed to a callee which requires a non-null
argument
      [-Wnonnull]
  test2(0);
  ^     ~

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110324/cf18e9ad/attachment.html>