On Wed, Jun 15, 2011 at 2:48 PM, Eli Friedman <[email protected]>wrote:
> On Wed, Jun 15, 2011 at 2:31 PM, Richard Trieu <[email protected]> wrote:
> > On Tue, Jun 14, 2011 at 7:10 AM, Douglas Gregor <[email protected]>
> wrote:
> >>
> >> On Jun 13, 2011, at 5:00 PM, Richard Trieu wrote:
> >>
> >>
> >> On Thu, May 26, 2011 at 3:10 AM, Chandler Carruth <[email protected]
> >
> >> wrote:
> >>>
> >>> On Mon, May 2, 2011 at 5:48 PM, Richard Trieu <[email protected]>
> wrote:
> >>>>
> >>>> Add warning when NULL constant value is used improperly in
> expressions.
> >>>
> >>> Some high-level comments:
> >>> 1) Why only handle GNUNull null-pointer-constants? I think there is a
> >>> good reason here (making them behave as much like nullptr as possible)
> but
> >>> it would be good to document that in comments at least.
> >>
> >> The other kinds of nulls are zero integer and c++0x nullptr.
> >> Integral zero is valid for these operations. nullptr is a different
> type
> >> which already produces an error in these cases.
> >>>
> >>> 2) What drives the set of invalid operations? I assume its those that
> >>> nullptr would be invalid for? If so, can you provide standard citations
> >>> against the draft standard? Also, is there no way to just turn on the
> C++0x
> >>> errors for nullptr when we see the GNUNull in C++03, but down-grade
> them to
> >>> warnings?
> >>
> >> These operations are the ones where the null pointer would be used as an
> >> integer instead of a pointer. I've also copied the test, but using
> nullptr
> >> instead to show that they error in same places.
> >> It should be possible to change the NULL into a nullptr and then run the
> >> checks, but that would probably involve touching code in all the of
> >> operation checking functions. I feel that it would be better to keep
> this
> >> code in one place instead of spread across so many functions.
> >>>
> >>> 3) Because these are warnings, we can't return ExprError; that changes
> >>> the parse result.
> >>
> >> Removed the early exit with ExprError.
> >>
> >> + bool LeftNull = Expr::NPCK_GNUNull ==
> >> + lhs.get()->isNullPointerConstant(Context,
> >> +
> >> Expr::NPC_ValueDependentIsNotNull);
> >> + bool RightNull = Expr::NPCK_GNUNull ==
> >> + rhs.get()->isNullPointerConstant(Context,
> >> +
> >> Expr::NPC_ValueDependentIsNotNull);
> >> +
> >> + // Detect when a NULL constant is used improperly in an expression.
> >> + if (LeftNull || RightNull) {
> >> I think you want:
> >> if (LeftNull != RightNull) {
> >
> > No, I did want (LeftNull || RightNull) since something like (NULL * NULL)
> > should be warned on. I will add the check so that using two NULLs will
> only
> > produce one warning. The inequality check is already present for
> comparison
> > operators.
> >>
> >> here? At the very least, we shouldn't emit two warnings when both sides
> of
> >> the operator are NULL.
> >> + // These are the operations that would not make sense with a null
> >> pointer
> >> + // if the other expression the other expression is not a pointer.
> >> + if ((LeftNull != RightNull) &&
> >> !lhs.get()->getType()->isPointerType() &&
> >> + !rhs.get()->getType()->isPointerType()) {
> >> You also need to check for member pointers, block pointers, and
> >> Objective-C pointers here.
> >> - Doug
> >
> > Two points here.
> > 1) In Objective-C test, I see that NULL is defined as (void*)0 which will
> > not cause it to be picked up by the NULL checks for GNU null. There are
> > already warnings for improper conversion to integer so having a special
> case
> > for Objective-C is not required.
>
> Not so for ObjC++.
>
> > 2) Should member pointers and block pointers behave the same as other
> > pointers for relational and quality operators with NULL and nullptr?
>
> Equality, yes. Relational operators with member and block pointers
> aren't legal.
>
> > For
> > NULL, the relational operators give an invalid operand error. For
> nullptr,
> > relational operators for both pointers and equality operators for block
> > pointers give an invalid operand error. This is different from other
> > pointers which will not complain. Is this proper behavior for these type
> of
> > pointers?
>
> Filed http://llvm.org/bugs/show_bug.cgi?id=10145 for the block
> pointer+nullptr thing.
>
> -Eli
>
Thanks for the help, Eli. Added the check for member, block, and
Objective-C pointers and tests for them. Fixed cases with two NULL's so
they only produce one warning instead of two.
Index: test/SemaObjCXX/null_objc_pointer.mm
===================================================================
--- test/SemaObjCXX/null_objc_pointer.mm (revision 0)
+++ test/SemaObjCXX/null_objc_pointer.mm (revision 0)
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+#import <stddef.h>
+
+@interface X
+@end
+
+void f() {
+ bool b;
+ X *d;
+ b = d < NULL || NULL < d || d > NULL || NULL > d;
+ b = d <= NULL || NULL <= d || d >= NULL || NULL >= d;
+ b = d == NULL || NULL == d || d != NULL || NULL != d;
+}
Index: test/SemaCXX/null_in_arithmetic_ops.cpp
===================================================================
--- test/SemaCXX/null_in_arithmetic_ops.cpp (revision 0)
+++ test/SemaCXX/null_in_arithmetic_ops.cpp (revision 0)
@@ -0,0 +1,70 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -verify %s
+#include <stddef.h>
+
+void f() {
+ int a;
+ bool b;
+
+ a = 0 ? NULL + a : a + NULL; // expected-warning 2{{use of NULL in arithmetic operation}}
+ a = 0 ? NULL - a : a - NULL; // expected-warning 2{{use of NULL in arithmetic operation}}
+ a = 0 ? NULL / a : a / NULL; // expected-warning 2{{use of NULL in arithmetic operation}} \
+ // expected-warning {{division by zero is undefined}}
+ a = 0 ? NULL * a : a * NULL; // expected-warning 2{{use of NULL in arithmetic operation}}
+ a = 0 ? NULL >> a : a >> NULL; // expected-warning 2{{use of NULL in arithmetic operation}}
+ a = 0 ? NULL << a : a << NULL; // expected-warning 2{{use of NULL in arithmetic operation}}
+ a = 0 ? NULL % a : a % NULL; // expected-warning 2{{use of NULL in arithmetic operation}} \
+ expected-warning {{remainder by zero is undefined}}
+ a = 0 ? NULL & a : a & NULL; // expected-warning 2{{use of NULL in arithmetic operation}}
+ a = 0 ? NULL | a : a | NULL; // expected-warning 2{{use of NULL in arithmetic operation}}
+ a = 0 ? NULL ^ a : a ^ NULL; // expected-warning 2{{use of NULL in arithmetic operation}}
+
+ // Using two NULLs should only give one error instead of two.
+ a = NULL + NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a = NULL - NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a = NULL / NULL; // expected-warning{{use of NULL in arithmetic operation}} \
+ // expected-warning{{division by zero is undefined}}
+ a = NULL * NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a = NULL >> NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a = NULL << NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a = NULL % NULL; // expected-warning{{use of NULL in arithmetic operation}} \
+ // expected-warning{{remainder by zero is undefined}}
+ a = NULL & NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a = NULL | NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a = NULL ^ NULL; // expected-warning{{use of NULL in arithmetic operation}}
+
+ a += NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a -= NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a /= NULL; // expected-warning{{use of NULL in arithmetic operation}} \
+ // expected-warning{{division by zero is undefined}}
+ a *= NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a >>= NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a <<= NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a %= NULL; // expected-warning{{use of NULL in arithmetic operation}} \
+ // expected-warning{{remainder by zero is undefined}}
+ a &= NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a |= NULL; // expected-warning{{use of NULL in arithmetic operation}}
+ a ^= NULL; // expected-warning{{use of NULL in arithmetic operation}}
+
+ b = a < NULL || NULL < a; // expected-warning 2{{use of NULL in arithmetic operation}}
+ b = a > NULL || NULL > a; // expected-warning 2{{use of NULL in arithmetic operation}}
+ b = a <= NULL || NULL <= a; // expected-warning 2{{use of NULL in arithmetic operation}}
+ b = a >= NULL || NULL >= a; // expected-warning 2{{use of NULL in arithmetic operation}}
+ b = a == NULL || NULL == a; // expected-warning 2{{use of NULL in arithmetic operation}}
+ b = a != NULL || NULL != a; // expected-warning 2{{use of NULL in arithmetic operation}}
+
+ b = &a < NULL || NULL < &a || &a > NULL || NULL > &a;
+ b = &a <= NULL || NULL <= &a || &a >= NULL || NULL >= &a;
+ b = &a == NULL || NULL == &a || &a != NULL || NULL != &a;
+
+ b = 0 == a;
+ b = 0 == &a;
+
+ b = ((NULL)) != a; // expected-warning{{use of NULL in arithmetic operation}}
+
+ void (^c)();
+ b = c == NULL || NULL == c || c != NULL || NULL != c;
+
+ class X;
+ void (X::*d) ();
+ b = d == NULL || NULL == d || d != NULL || NULL != d;
+}
Index: test/SemaCXX/nullptr_in_arithmetic_ops.cpp
===================================================================
--- test/SemaCXX/nullptr_in_arithmetic_ops.cpp (revision 0)
+++ test/SemaCXX/nullptr_in_arithmetic_ops.cpp (revision 0)
@@ -0,0 +1,67 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -std=c++0x -verify %s
+
+void f() {
+ int a;
+ bool b;
+
+ a = 0 ? nullptr + a : a + nullptr; // expected-error 2{{invalid operands to binary expression}}
+ a = 0 ? nullptr - a : a - nullptr; // expected-error 2{{invalid operands to binary expression}}
+ a = 0 ? nullptr / a : a / nullptr; // expected-error 2{{invalid operands to binary expression}}
+ a = 0 ? nullptr * a : a * nullptr; // expected-error 2{{invalid operands to binary expression}}
+ a = 0 ? nullptr >> a : a >> nullptr; // expected-error 2{{invalid operands to binary expression}}
+ a = 0 ? nullptr << a : a << nullptr; // expected-error 2{{invalid operands to binary expression}}
+ a = 0 ? nullptr % a : a % nullptr; // expected-error 2{{invalid operands to binary expression}}
+ a = 0 ? nullptr & a : a & nullptr; // expected-error 2{{invalid operands to binary expression}}
+ a = 0 ? nullptr | a : a | nullptr; // expected-error 2{{invalid operands to binary expression}}
+ a = 0 ? nullptr ^ a : a ^ nullptr; // expected-error 2{{invalid operands to binary expression}}
+
+ // Using two nullptrs should only give one error instead of two.
+ a = nullptr + nullptr; // expected-error{{invalid operands to binary expression}}
+ a = nullptr - nullptr; // expected-error{{invalid operands to binary expression}}
+ a = nullptr / nullptr; // expected-error{{invalid operands to binary expression}}
+ a = nullptr * nullptr; // expected-error{{invalid operands to binary expression}}
+ a = nullptr >> nullptr; // expected-error{{invalid operands to binary expression}}
+ a = nullptr << nullptr; // expected-error{{invalid operands to binary expression}}
+ a = nullptr % nullptr; // expected-error{{invalid operands to binary expression}}
+ a = nullptr & nullptr; // expected-error{{invalid operands to binary expression}}
+ a = nullptr | nullptr; // expected-error{{invalid operands to binary expression}}
+ a = nullptr ^ nullptr; // expected-error{{invalid operands to binary expression}}
+
+ a += nullptr; // expected-error{{invalid operands to binary expression}}
+ a -= nullptr; // expected-error{{invalid operands to binary expression}}
+ a /= nullptr; // expected-error{{invalid operands to binary expression}}
+ a *= nullptr; // expected-error{{invalid operands to binary expression}}
+ a >>= nullptr; // expected-error{{invalid operands to binary expression}}
+ a <<= nullptr; // expected-error{{invalid operands to binary expression}}
+ a %= nullptr; // expected-error{{invalid operands to binary expression}}
+ a &= nullptr; // expected-error{{invalid operands to binary expression}}
+ a |= nullptr; // expected-error{{invalid operands to binary expression}}
+ a ^= nullptr; // expected-error{{invalid operands to binary expression}}
+
+ b = a < nullptr || nullptr < a; // expected-error 2{{invalid operands to binary expression}}
+ b = a > nullptr || nullptr > a; // expected-error 2{{invalid operands to binary expression}}
+ b = a <= nullptr || nullptr <= a; // expected-error 2{{invalid operands to binary expression}}
+ b = a >= nullptr || nullptr >= a; // expected-error 2{{invalid operands to binary expression}}
+ b = a == nullptr || nullptr == a; // expected-error 2{{invalid operands to binary expression}}
+ b = a != nullptr || nullptr != a; // expected-error 2{{invalid operands to binary expression}}
+
+ b = &a < nullptr || nullptr < &a || &a > nullptr || nullptr > &a;
+ b = &a <= nullptr || nullptr <= &a || &a >= nullptr || nullptr >= &a;
+ b = &a == nullptr || nullptr == &a || &a != nullptr || nullptr != &a;
+
+ b = 0 == a;
+ b = 0 == &a;
+
+ b = ((nullptr)) != a; // expected-error{{invalid operands to binary expression}}
+
+ /* FIXME: This is blocked on PR10145
+ void (^c)();
+ c = nullptr;
+ b = c == nullptr || nullptr == c || c != nullptr || nullptr != c;
+ */
+
+ class X;
+ void (X::*d) ();
+ d = nullptr;
+ b = d == nullptr || nullptr == d || d != nullptr || nullptr != d;
+}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 130739)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -2699,6 +2699,8 @@
def warn_comparison_of_mixed_enum_types : Warning<
"comparison of two values with different enumeration types (%0 and %1)">,
InGroup<DiagGroup<"enum-compare">>;
+def warn_null_in_arithmetic_operation : Warning<
+ "use of NULL in arithmetic operation">;
def err_invalid_this_use : Error<
"invalid use of 'this' outside of a nonstatic member function">;
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 130739)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -8570,6 +8570,60 @@
rhs = move(resolvedRHS);
}
+ bool LeftNull = Expr::NPCK_GNUNull ==
+ lhs.get()->isNullPointerConstant(Context,
+ Expr::NPC_ValueDependentIsNotNull);
+ bool RightNull = Expr::NPCK_GNUNull ==
+ rhs.get()->isNullPointerConstant(Context,
+ Expr::NPC_ValueDependentIsNotNull);
+
+ // Detect when a NULL constant is used improperly in an expression. These
+ // are mainly cases where the null pointer is used as an integer instead
+ // of a pointer.
+ if (LeftNull || RightNull) {
+ if (Opc == BO_Mul || Opc == BO_Div || Opc == BO_Rem || Opc == BO_Add ||
+ Opc == BO_Sub || Opc == BO_Shl || Opc == BO_Shr || Opc == BO_And ||
+ Opc == BO_Xor || Opc == BO_Or || Opc == BO_MulAssign ||
+ Opc == BO_DivAssign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
+ Opc == BO_RemAssign || Opc == BO_ShlAssign || Opc == BO_ShrAssign ||
+ Opc == BO_AndAssign || Opc == BO_OrAssign || Opc == BO_XorAssign) {
+ // These are the operations that would not make sense with a null pointer
+ // no matter what the other expression is.
+ if (LeftNull && RightNull) {
+ Diag(OpLoc, diag::warn_null_in_arithmetic_operation)
+ << lhs.get()->getSourceRange() << rhs.get()->getSourceRange();
+ } else if (LeftNull) {
+ Diag(OpLoc, diag::warn_null_in_arithmetic_operation)
+ << lhs.get()->getSourceRange();
+ } else if (RightNull) {
+ Diag(OpLoc, diag::warn_null_in_arithmetic_operation)
+ << rhs.get()->getSourceRange();
+ }
+ } else if (Opc == BO_LE || Opc == BO_LT || Opc == BO_GE || Opc == BO_GT ||
+ Opc == BO_EQ || Opc == BO_NE) {
+ // These are the operations that would not make sense with a null pointer
+ // if the other expression the other expression is not a pointer.
+ QualType LeftType = lhs.get()->getType();
+ QualType RightType = rhs.get()->getType();
+ bool LeftPointer = LeftType->isPointerType() ||
+ LeftType->isBlockPointerType() ||
+ LeftType->isMemberPointerType() ||
+ LeftType->isObjCObjectPointerType();
+ bool RightPointer = RightType->isPointerType() ||
+ RightType->isBlockPointerType() ||
+ RightType->isMemberPointerType() ||
+ RightType->isObjCObjectPointerType();
+ if ((LeftNull != RightNull) && !LeftPointer && !RightPointer) {
+ if (LeftNull)
+ Diag(OpLoc, diag::warn_null_in_arithmetic_operation)
+ << lhs.get()->getSourceRange();
+ if (RightNull)
+ Diag(OpLoc, diag::warn_null_in_arithmetic_operation)
+ << rhs.get()->getSourceRange();
+ }
+ }
+ }
+
switch (Opc) {
case BO_Assign:
ResultTy = CheckAssignmentOperands(lhs.get(), rhs, OpLoc, QualType());
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
