r285856 - Don't require nullability on template parameters in typedefs.

[Richard Smith via cfe-commits]

On Wed, Nov 2, 2016 at 3:51 PM, Jordan Rose via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

>
> On Nov 2, 2016, at 15:48, Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Wed, Nov 2, 2016 at 2:34 PM, Jordan Rose via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>>
>> On Nov 2, 2016, at 14:31, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> On 2 Nov 2016 1:53 pm, "Jordan Rose via cfe-commits" <
>> cfe-commits at lists.llvm.org> wrote:
>>
>> Author: jrose
>> Date: Wed Nov  2 15:44:07 2016
>> New Revision: 285856
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=285856&view=rev
>> Log:
>> Don't require nullability on template parameters in typedefs.
>>
>> Previously the following code would warn on the use of "T":
>>
>>   template <typename T>
>>   struct X {
>>     typedef T *type;
>>   };
>>
>> ...because nullability is /allowed/ on template parameters (because
>> they could be pointers). (Actually putting nullability on this use of
>> 'T' will of course break if the argument is a non-pointer type.)
>>
>>
>> This doesn't make any sense to me. Why would T need to be a pointer type
>> for a nullability qualifier to be valid on a T*?
>>
>>
>> Sorry, this is referring to the following change to the example:
>>
>> template <typename T>
>> struct X {
>>   typedef T _Nullable *type;
>> };
>>
>>
>> This is legal, but of course `X<int>` then produces an error. So we want
>> to accept nullability in this position (in case T is implicitly required to
>> be a pointer type by the definition of X) but not warn when it’s missing
>> (in case it isn’t).
>>
>
> Oh, I see. Your testcase is very confusing, though, since it wraps the
> problematic use of T in a completely-unrelated pointer type. The actual
> problem being fixed is much more obvious in a case like this:
>
> int *_Nullable p;
> template<typename T> struct X {
>   T t; // warns without your fix
> };
>
> It'd be easier on future readers of this code to use the more obvious test
> case here.
>
>
> Ah, that case doesn’t actually trigger the issue, because a typedef
> doesn’t require nullability on its outermost pointer type. (It’s assumed
> that the use site may need to make some uses of the typedef nullable and
> some non-nullable.)
>

The testcase I gave above definitely produces a bogus warning before your
patch. I've not actually checked whether the patch fixes it, though.


> We *do* still see this issue for *fields* of type ’T’, but that seemed
> trickier to deal with. I might have been overthinking it, though.
>
> Jordan
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161102/4570e32f/attachment.html>