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