<div dir="ltr"><span style="font-size:12.8px">> Are there already tests in place for the C version? I assume so, but</span><div><span style="font-size:12.8px">> please double check.</span><br></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">Looks to me like the warning in the example is tested in C. Added 2</span></div><div><span style="font-size:12.8px">other cases (assignment + passing a _Nullable variable as a </span></div><div><span style="font-size:12.8px">_Nonnull argument) for good measure.</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">> </span><span style="font-size:12.8px">LGTM with a couple of nits, below.</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">All done. Thanks for the review! :)</span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 14, 2015 at 1:20 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">George Burgess IV via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> writes:<br>
> george.burgess.iv created this revision.<br>
> george.burgess.iv added a reviewer: doug.gregor.<br>
> george.burgess.iv added subscribers: cfe-commits, doug.gregor.<br>
><br>
> Currently, code like this compiles cleanly in C++, but with warnings<br>
> (as it should) in C:<br>
<br>
</span>Are there already tests in place for the C version? I assume so, but<br>
please double check.<br>
<span class=""><br>
><br>
> ```<br>
> int foo() {<br>
> void *_Nullable p;<br>
> void *_Nonnull n = p;<br>
> }<br>
> ```<br>
><br>
> This patch makes us emit the appropriate warnings in C++.<br>
<br>
</span>LGTM with a couple of nits, below.<br>
<span class=""><br>
> @doug.gregor: `arc` said you would be best to review this; if you're<br>
> unable to, I'm happy to ping others. :)<br>
><br>
> <a href="http://reviews.llvm.org/D14938" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14938</a><br>
><br>
> Files:<br>
> include/clang/Sema/Sema.h<br>
> lib/Sema/Sema.cpp<br>
> lib/Sema/SemaExpr.cpp<br>
> lib/Sema/SemaExprCXX.cpp<br>
> test/SemaCXX/nullability.cpp<br>
><br>
</span>> Index: test/SemaCXX/nullability.cpp<br>
> ===================================================================<br>
> --- test/SemaCXX/nullability.cpp<br>
> +++ test/SemaCXX/nullability.cpp<br>
> @@ -1,4 +1,4 @@<br>
> -// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wno-nullability-declspec %s -verify<br>
> +// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wno-nullability-declspec %s -verify -Wnullable-to-nonnull-conversion<br>
><br>
> #if __has_feature(nullability)<br>
> #else<br>
> @@ -67,3 +67,33 @@<br>
> }<br>
><br>
> template void test_accepts_nonnull_null_pointer_literal_template<&accepts_nonnull_4>(); // expected-note{{instantiation of function template specialization}}<br>
> +<br>
> +void TakeNonnull(void *_Nonnull);<br>
> +// Check different forms of assignment to a nonull type from a nullable one.<br>
> +void AssignAndInitNonNull() {<br>
> + void *_Nullable nullable;<br>
> + void *_Nonnull p(nullable); // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> + void *_Nonnull p2{nullable}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> + void *_Nonnull p3 = {nullable}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> + void *_Nonnull p4 = nullable; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> + void *_Nonnull nonnull;<br>
> + nonnull = nullable; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> + nonnull = {nullable}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> +<br>
> + TakeNonnull(nullable); //expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull}}<br>
> + TakeNonnull(nonnull); // OK<br>
> +}<br>
> +<br>
> +void *_Nullable ReturnNullable();<br>
> +<br>
> +void AssignAndInitNonNullFromFn() {<br>
> + void *_Nonnull p(ReturnNullable()); // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> + void *_Nonnull p2{ReturnNullable()}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> + void *_Nonnull p3 = {ReturnNullable()}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> + void *_Nonnull p4 = ReturnNullable(); // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> + void *_Nonnull nonnull;<br>
> + nonnull = ReturnNullable(); // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> + nonnull = {ReturnNullable()}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}<br>
> +<br>
> + TakeNonnull(ReturnNullable()); //expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull}}<br>
> +}<br>
> Index: lib/Sema/SemaExprCXX.cpp<br>
> ===================================================================<br>
> --- lib/Sema/SemaExprCXX.cpp<br>
> +++ lib/Sema/SemaExprCXX.cpp<br>
> @@ -3118,6 +3118,7 @@<br>
> ToType = ToAtomic->getValueType();<br>
> }<br>
><br>
> + QualType InitialFromType = FromType;<br>
> // Perform the first implicit conversion.<br>
> switch (SCS.First) {<br>
> case ICK_Identity:<br>
> @@ -3488,6 +3489,12 @@<br>
> VK_RValue, nullptr, CCK).get();<br>
> }<br>
><br>
> + // If this conversion sequence succeeded and involved implicitly converting a<br>
> + // _Nullable type to a _Nonnull one, complain.<br>
> + if (CCK == CCK_ImplicitConversion)<br>
> + diagnoseNullableToNonnullConversion(ToType, InitialFromType,<br>
> + From->getLocStart());<br>
> +<br>
> return From;<br>
> }<br>
><br>
> Index: lib/Sema/SemaExpr.cpp<br>
> ===================================================================<br>
> --- lib/Sema/SemaExpr.cpp<br>
> +++ lib/Sema/SemaExpr.cpp<br>
> @@ -11823,8 +11823,8 @@<br>
><br>
> switch (ConvTy) {<br>
> case Compatible:<br>
> - DiagnoseAssignmentEnum(DstType, SrcType, SrcExpr);<br>
> - return false;<br>
> + DiagnoseAssignmentEnum(DstType, SrcType, SrcExpr);<br>
> + return false;<br>
<br>
This looks unrelated / please commit formatting cleanup separately.<br>
<br>
><br>
> case PointerToInt:<br>
> DiagKind = diag::ext_typecheck_convert_pointer_int;<br>
> Index: lib/Sema/Sema.cpp<br>
> ===================================================================<br>
> --- lib/Sema/Sema.cpp<br>
> +++ lib/Sema/Sema.cpp<br>
> @@ -349,6 +349,16 @@<br>
> AnalysisWarnings.PrintStats();<br>
> }<br>
><br>
> +void Sema::diagnoseNullableToNonnullConversion(QualType DstType,<br>
> + QualType SrcType,<br>
> + SourceLocation Loc) {<br>
> + if (auto ExprNullability = SrcType->getNullability(Context))<br>
> + if (*ExprNullability == NullabilityKind::Nullable)<br>
> + if (auto TypeNullability = DstType->getNullability(Context))<br>
> + if (*TypeNullability == NullabilityKind::NonNull)<br>
> + Diag(Loc, diag::warn_nullability_lost) << SrcType << DstType;<br>
> +}<br>
<br>
This might be more readable with an early return or two. If it doesn't<br>
seem any more readable to you it's fine as is though.<br>
<br>
> +<br>
> /// ImpCastExprToType - If Expr is not of type 'Type', insert an implicit cast.<br>
> /// If there is already an implicit cast, merge into the existing one.<br>
> /// The result is of the given category.<br>
> @@ -372,18 +382,7 @@<br>
> assert((VK == VK_RValue || !E->isRValue()) && "can't cast rvalue to lvalue");<br>
> #endif<br>
><br>
> - // Check whether we're implicitly casting from a nullable type to a nonnull<br>
> - // type.<br>
> - if (auto exprNullability = E->getType()->getNullability(Context)) {<br>
> - if (*exprNullability == NullabilityKind::Nullable) {<br>
> - if (auto typeNullability = Ty->getNullability(Context)) {<br>
> - if (*typeNullability == NullabilityKind::NonNull) {<br>
> - Diag(E->getLocStart(), diag::warn_nullability_lost)<br>
> - << E->getType() << Ty;<br>
> - }<br>
> - }<br>
> - }<br>
> - }<br>
> + diagnoseNullableToNonnullConversion(Ty, E->getType(), E->getLocStart());<br>
><br>
> QualType ExprTy = Context.getCanonicalType(E->getType());<br>
> QualType TypeTy = Context.getCanonicalType(Ty);<br>
> Index: include/clang/Sema/Sema.h<br>
> ===================================================================<br>
> --- include/clang/Sema/Sema.h<br>
> +++ include/clang/Sema/Sema.h<br>
> @@ -3505,6 +3505,11 @@<br>
> void DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,<br>
> SourceLocation OpLoc);<br>
><br>
> + /// \brief Warn if we're implicitly casting from a _Nullable pointer type to a<br>
> + /// _Nonnull one.<br>
> + void diagnoseNullableToNonnullConversion(QualType DstType, QualType SrcType,<br>
> + SourceLocation Loc);<br>
> +<br>
> ParsingDeclState PushParsingDeclaration(sema::DelayedDiagnosticPool &pool) {<br>
> return DelayedDiagnostics.push(pool);<br>
> }<br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>