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

[Richard Smith via cfe-commits]

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.

Jordan
>
>
> This fix doesn't handle the case where a template parameter is used
> /outside/ of a typedef. That seems trickier, especially in parameter
> position.
>
> Modified:
>     cfe/trunk/lib/Sema/SemaType.cpp
>     cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-1.h
>
> Modified: cfe/trunk/lib/Sema/SemaType.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaT
> ype.cpp?rev=285856&r1=285855&r2=285856&view=diff
> ============================================================
> ==================
> --- cfe/trunk/lib/Sema/SemaType.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaType.cpp Wed Nov  2 15:44:07 2016
> @@ -3600,7 +3600,17 @@ static TypeSourceInfo *GetFullTypeForDec
>      // inner pointers.
>      complainAboutMissingNullability = CAMN_InnerPointers;
>
> -    if (T->canHaveNullability() && !T->getNullability(S.Context)) {
> +    auto isDependentNonPointerType = [](QualType T) -> bool {
> +      // Note: This is intended to be the same check as
> Type::canHaveNullability
> +      // except with all of the ambiguous cases being treated as 'false'
> rather
> +      // than 'true'.
> +      return T->isDependentType() && !T->isAnyPointerType() &&
> +        !T->isBlockPointerType() && !T->isMemberPointerType();
> +    };
> +
> +    if (T->canHaveNullability() && !T->getNullability(S.Context) &&
> +        !isDependentNonPointerType(T)) {
> +      // Note that we allow but don't require nullability on dependent
> types.
>        ++NumPointersRemaining;
>      }
>
>
> Modified: cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-1.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjCX
> X/Inputs/nullability-consistency-1.h?rev=285856&r1=285855&
> r2=285856&view=diff
> ============================================================
> ==================
> --- cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-1.h
> (original)
> +++ cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-1.h Wed Nov
> 2 15:44:07 2016
> @@ -13,5 +13,13 @@ class X {
>    int X:: *memptr; // expected-warning{{member pointer is missing a
> nullability type specifier}}
>  };
>
> +template <typename T>
> +struct Typedefs {
> +  typedef T *Base; // no-warning
> +  typedef Base *type; // expected-warning{{pointer is missing a
> nullability type specifier}}
> +};
> +
> +Typedefs<int> xx;
> +Typedefs<void *> yy;
>
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
>
> _______________________________________________
> 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/8bd8dd6c/attachment-0001.html>