[PATCH] D21298: [Clang-tidy] delete null check

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Thu Nov 17 15:21:10 PST 2016


aaron.ballman added inline comments.


================
Comment at: test/clang-tidy/readability-delete-null-pointer.cpp:7
+  int *p = 0;
+  if (p) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'if' statement is unnecessary; deleting null pointer has no effect [readability-delete-null-pointer]
----------------
SilverGeri wrote:
> aaron.ballman wrote:
> > hokein wrote:
> > > Does it work the case like:
> > > 
> > > ```
> > > int *p = nullptr;
> > > if (p == nullptr) {
> > >    p = new int[3];
> > >    delete[] p;
> > > }
> > > ```
> > > 
> > > ?
> > Similarly, it should not mishandle a case like:
> > 
> > void f(int *p) {
> >   if (p) {
> >     delete p;
> >   } else {
> >     // Do something else
> >   }
> > }
> it warns only if the compund statement contains only one statement (which is the delete). We want to warn because it is unnecessary to check the pointer validity if you want to just call `delete`. In other cases, we can't be sure about the actual behaviour.
In my example, the compound statement does contain only one statement, the delete, but diagnosing is likely a false positive due to the else clause. In that case, the pointer validity controls more than just the delete because it also controls whether to execute the else clause. Removing the if clause shouldn't be a mechanical change in the presence of an else clause, so the fixit is definitely inappropriate. I think that diagnosing is still pretty reasonable, however.

Another test case, which I think may already be handled appropriately but should still be explicitly tested:
```
if (p) {
  // This comment should not disable the check or the fixit.
  // Nor should this comment.
  delete p;
}
```
I think this check should still be able to remove the if clause, but we should make sure that the comments don't disable the check, and that the fixit doesn't destroy the comments.


https://reviews.llvm.org/D21298





More information about the cfe-commits mailing list