[cfe-commits] Patch: Add a warning for NULL used in arithmetic operations

[Douglas Gregor]

On Jun 13, 2011, at 5:00 PM, Richard Trieu wrote:

> 
> 
> On Thu, May 26, 2011 at 3:10 AM, Chandler Carruth <chandlerc at google.com> wrote:
> On Mon, May 2, 2011 at 5:48 PM, Richard Trieu <rtrieu at google.com> 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) {

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110614/cd502fa2/attachment.html>