r199663 - Making some minor improvements to r199626.

[Aaron Ballman]

On Fri, Jul 11, 2014 at 12:13 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
>> From: "Aaron Ballman" <aaron at aaronballman.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>, "Ted Kremenek" <kremenek at apple.com>
>> Cc: "llvm cfe" <cfe-commits at cs.uiuc.edu>
>> Sent: Friday, July 11, 2014 11:05:30 AM
>> Subject: Re: r199663 - Making some minor improvements to r199626.
>>
>> On Fri, Jul 11, 2014 at 10:48 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>> > ----- Original Message -----
>> >> From: "Aaron Ballman" <aaron at aaronballman.com>
>> >> To: cfe-commits at cs.uiuc.edu
>> >> Sent: Monday, January 20, 2014 8:19:44 AM
>> >> Subject: r199663 - Making some minor improvements to r199626.
>> >>
>> >> Author: aaronballman
>> >> Date: Mon Jan 20 08:19:44 2014
>> >> New Revision: 199663
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=199663&view=rev
>> >> Log:
>> >> Making some minor improvements to r199626.
>> >>
>> >> Modified:
>> >>     cfe/trunk/include/clang/Basic/Attr.td
>> >>     cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>> >>     cfe/trunk/test/Sema/nonnull.c
>> >>
>> >
>> > [snip]
>> >
>> >> Modified: cfe/trunk/test/Sema/nonnull.c
>> >> URL:
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/nonnull.c?rev=199663&r1=199662&r2=199663&view=diff
>> >> ==============================================================================
>> >> --- cfe/trunk/test/Sema/nonnull.c (original)
>> >> +++ cfe/trunk/test/Sema/nonnull.c Mon Jan 20 08:19:44 2014
>> >> @@ -32,10 +32,10 @@ void test_baz() {
>> >>    baz3(0); // no-warning
>> >>  }
>> >>
>> >> -void test_void_returns_nonnull()
>> >> __attribute__((returns_nonnull));
>> >> // expected-warning {{'returns_nonnull' attribute only applies to
>> >> return values that are pointers}}
>> >> -int test_int_returns_nonnull() __attribute__((returns_nonnull));
>> >> //
>> >> expected-warning {{'returns_nonnull' attribute only applies to
>> >> return values that are pointers}}
>> >> -void *test_ptr_returns_nonnull()
>> >> __attribute__((returns_nonnull));
>> >> // no-warning
>> >> +void test_void_returns_nonnull(void)
>> >> __attribute__((returns_nonnull)); // expected-warning
>> >> {{'returns_nonnull' attribute only applies to return values that
>> >> are
>> >> pointers}}
>> >> +int test_int_returns_nonnull(void)
>> >> __attribute__((returns_nonnull));
>> >> // expected-warning {{'returns_nonnull' attribute only applies to
>> >> return values that are pointers}}
>> >> +void *test_ptr_returns_nonnull(void)
>> >> __attribute__((returns_nonnull)); // no-warning
>> >>
>> >>  int i __attribute__((nonnull)); // expected-warning {{'nonnull'
>> >>  attribute only applies to functions, methods, and parameters}}
>> >>  int j __attribute__((returns_nonnull)); // expected-warning
>> >>  {{'returns_nonnull' attribute only applies to functions and
>> >>  methods}}
>> >> -
>> >> +void *test_no_fn_proto() __attribute__((returns_nonnull));  //
>> >> expected-warning {{'returns_nonnull' attribute only applies to
>> >> functions and methods}}
>> >
>> > This last check seems wrong to me. There is now a difference
>> > between this:
>> >
>> > void *test_ptr_returns_nonnull(void)
>> > __attribute__((returns_nonnull));
>> >
>> > which we handle and this:
>> >
>> > void *test_ptr_returns_nonnull() __attribute__((returns_nonnull));
>> >
>> > for which we ignore the attribute. However:
>> >
>> >  - Most people don't put void in the parameter lists, and
>>
>> This is a C test file, where () is different than (void). The former
>> has no prototype and is almost equivalent to it being written (...),
>> while the latter has a prototype accepting no arguments. In C++ mode,
>> there is no distinction.
>
> I understand the distinction, but it is not relevant to an attribute on the return value.

Agreed.

>
>>
>> >  - gcc 4.9.0 does not distinguish between the two forms w.r.t. the
>> >  application of this attribute.
>> >
>> > Can you please fix this?
>>
>> Ted was the original author of this functionality
>> (http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140113/097562.html),
>> this commit was a tidying of his original code but had no functional
>> changes. Are you asking to have C code with no function prototype
>> honor this attribute?
>
> Yes, this is what gcc does. Also, you clearly did change the behavior in Ted's test case because you removed this:
>
> -void *test_ptr_returns_nonnull() __attribute__((returns_nonnull)); // no-warning
>
> and then added this:
>
> +void *test_no_fn_proto() __attribute__((returns_nonnull));  // expected-warning {{'returns_nonnull' attribute only applies to functions and methods}}

Derp! I misspoke, sorry about that.

Ted's original declaration of the attribute set a subject requirement
of "HasFunctionProto", which is I was asking for Ted's opinion in case
there was logic there I wasn't following. That being said, I don't see
why this would require a prototype (and gcc compat makes sense too).
I'll look into changing this, unless Ted comes back with something.

~Aaron