This is an archive of the discontinued LLVM Phabricator instance.

[Clang-tidy] delete null check
ClosedPublic

Authored by SilverGeri on Jun 13 2016, 9:40 AM.

Details

Summary

removes the unnecessary if statements, if it was used to check a pointer's validity just to call delete on that pointer

Diff Detail

Event Timeline

SilverGeri retitled this revision from to delete null check.
SilverGeri updated this object.
SilverGeri added a reviewer: xazax.hun.
SilverGeri added a subscriber: cfe-commits.
Eugene.Zelenko retitled this revision from delete null check to [Clang-tidy] delete null check.Jun 13 2016, 10:27 AM
Eugene.Zelenko set the repository for this revision to rL LLVM.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

I think misc-delete-null-pointer will be better name.

Will be good idea to change some if statements in regression test to (p != nullptr) (for C++11) and (p != NULL) (pre-C+11). See http://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-cast.html.

etienneb edited edge metadata.Jun 13 2016, 10:41 AM

Some comments after a quick look to the code.

clang-tidy/misc/DeleteNullCheck.cpp
22 ↗(On Diff #60549)

HasDeleteExpr -> DeleteExpr

23 ↗(On Diff #60549)

The use of hasDescendant is Expensive.
There is cast to ignore? You could probably just skip cast/parens.

If the intend of this match-statement is to match comparison against NULL, it should be expanded to be more precise.

47 ↗(On Diff #60549)

This check could be moved to the matcher part above/
see ASTMatcher: hasCastKind

48 ↗(On Diff #60549)

see: equalsBoundNode

49 ↗(On Diff #60549)

This check should be moved to the matcher part too.
see: statementCountIs

aaron.ballman added inline comments.Jun 13 2016, 10:47 AM
clang-tidy/misc/DeleteNullCheck.cpp
23 ↗(On Diff #60549)

I think you want to use has() instead of hasDescendant() here. Otherwise, I think this code may trigger on delete foo(some_declref) where foo() returns a pointer. You may also need to ignore implicit casts here.

35 ↗(On Diff #60549)

Won't this trigger on code like if (foo && bar) delete foo->bar;? It doesn't look like you handle that sort of case below.

alexfh requested changes to this revision.Jun 16 2016, 7:37 AM
alexfh edited edge metadata.Jun 16 2016, 7:37 AM
This revision now requires changes to proceed.Jun 16 2016, 7:37 AM
dkrupp added a subscriber: dkrupp.Sep 13 2016, 9:47 AM
SilverGeri edited edge metadata.
SilverGeri removed rL LLVM as the repository for this revision.
Eugene.Zelenko added inline comments.Nov 2 2016, 4:25 PM
docs/clang-tidy/checks/misc-delete-null-pointer.rst
10 ↗(On Diff #76803)

Please indent variable declaration.

SilverGeri edited edge metadata.

I guess, "readability" will be a better category for this check.

checks if (p != 0) conditions, too

aaron.ballman added inline comments.Nov 10 2016, 8:27 AM
clang-tidy/misc/DeleteNullPointerCheck.cpp
54 ↗(On Diff #77015)

Rather than a parenthetical, I would just use a semicolon to distinguish the clauses. Also, I would quote 'if' to make it clear that you're talking about syntax rather than an English term.

clang-tidy/misc/DeleteNullPointerCheck.h
19 ↗(On Diff #77015)

You should write that short description.

Prazek added a subscriber: Prazek.Nov 10 2016, 11:18 PM
Prazek added inline comments.
test/clang-tidy/misc-delete-null-pointer.cpp
11 ↗(On Diff #77015)

Is there check-fixes-not? This seems to be required here, because even if the fixit won't happen here, the test will pass.

move checker to readability module
add missing description to header file

update tests with "CHECK-FIXES-NOT" parts

hokein added a subscriber: hokein.Nov 17 2016, 2:37 AM
hokein added inline comments.
test/clang-tidy/readability-delete-null-pointer.cpp
4

We don't rely on implementations of standard headers in lit test. You should fake the function/class that you need in this test.

8

Does it work the case like:

int *p = nullptr;
if (p == nullptr) {
   p = new int[3];
   delete[] p;
}

?

aaron.ballman added inline comments.Nov 17 2016, 5:48 AM
test/clang-tidy/readability-delete-null-pointer.cpp
8

Similarly, it should not mishandle a case like:

void f(int *p) {

if (p) {
  delete p;
} else {
  // Do something else
}

}

77

Please add a newline.

SilverGeri added inline comments.Nov 17 2016, 2:02 PM
test/clang-tidy/readability-delete-null-pointer.cpp
8

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.

aaron.ballman added inline comments.Nov 17 2016, 3:21 PM
test/clang-tidy/readability-delete-null-pointer.cpp
8

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.

only warn, not fix when the 'if' statement has 'else' clause
keeping comments

SilverGeri marked 4 inline comments as done.Nov 26 2016, 10:29 AM
hokein accepted this revision.Dec 1 2016, 2:13 AM
hokein added a reviewer: hokein.

It looks good to me, but you'd better wait for the approval from @aaron.ballman.

clang-tidy/readability/DeleteNullPointerCheck.cpp
56

I would use an early return if (IfWithDelete->getElse()) return here.

alexfh requested changes to this revision.Dec 13 2016, 7:08 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/readability/DeleteNullPointerCheck.cpp
53

Rename D to Diag, please.

56

Definitely use early exit.

57

This variable is unused.

73–77

Please clang-format the file.

test/clang-tidy/readability-delete-null-pointer.cpp
21

Please truncate repeated static parts of the check patterns that exceed 80 characters (e.g. remove the deleting null pointer has no effect [readability-delete-null-pointer] part from all but the first CHECK line).

54

Please add CHECK-FIXES lines. Now there's no easy way to see from the test whether any fixes are applied here.

This revision now requires changes to proceed.Dec 13 2016, 7:08 AM
SilverGeri edited edge metadata.

remove unused string
using early exit in condition
shorten check-message lines
add check-fisex to 'else' part

SilverGeri planned changes to this revision.Dec 14 2016, 1:28 PM
SilverGeri marked 6 inline comments as done.
SilverGeri marked an inline comment as done.
SilverGeri edited edge metadata.
Prazek added inline comments.Dec 14 2016, 3:07 PM
clang-tidy/readability/DeleteNullPointerCheck.cpp
34

I think allOf matcher is redundant here because ifStmt takes variadic number of arguments and matches only if all of them matches.

removing redundant allOf from ifStmt

SilverGeri marked an inline comment as done.Dec 16 2016, 1:12 AM
aaron.ballman accepted this revision.Dec 19 2016, 9:29 AM
aaron.ballman edited edge metadata.

LGTM, with one nit. You should wait for @alexfh to sign off before committing though, since he requested changes.

clang-tidy/readability/DeleteNullPointerCheck.cpp
54

Elide the braces.

SilverGeri edited edge metadata.

remove brackets

SilverGeri marked an inline comment as done.Dec 23 2016, 1:38 AM
xazax.hun edited edge metadata.Dec 29 2016, 2:45 AM

Small ping, is this ready to be committed?

Small ping, is this ready to be committed?

If @alexfh doesn't sign off by tomorrow, I think it's fine to commit. We can deal with any last minute comments post-commit.

alexfh requested changes to this revision.Dec 29 2016, 8:54 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/readability/DeleteNullPointerCheck.cpp
30

Since binaryOperator (and most if not all other node matchers) is VariadicDynCastAllOfMatcher, allOf is redundant here (similar to Piotr's comment below).

Same for compoundStmt below.

docs/clang-tidy/checks/readability-delete-null-pointer.rst
7

Enclose if in double backquotes instead of single quotes.

8

Either null pointer or nullptr (enclosed in double backquotes).

11

Insert an empty line above and check that Sphinx can build this.

test/clang-tidy/readability-delete-null-pointer.cpp
8

This check (also combined with the ones below) is useless. It will work for the unchanged file as well: it will skip the if (p) line and find the comment, the next CHECK-FIXES will find the delete p; line and the CHECK-FIXES-NOT will find no lines matching if (p) between them.

I'd use something like this:

// comment1
if (p) { // comment 2
  delete p;
} // comment 3
// CHECK-FIXES: {{^  }}// comment1
// CHECK-FIXES-NEXT: {{^  }} // comment2
// CHECK-FIXES-NEXT: {{^  }}  delete p;
// CHECK-FIXES-NEXT: {{^  }} // comment3

Same for patterns below.

This revision now requires changes to proceed.Dec 29 2016, 8:54 AM

Small ping, is this ready to be committed?

If @alexfh doesn't sign off by tomorrow, I think it's fine to commit. We can deal with any last minute comments post-commit.

Yup, that's totally fine, especially when there was a thorough pre-commit code review.

Small ping, is this ready to be committed?

If @alexfh doesn't sign off by tomorrow, I think it's fine to commit. We can deal with any last minute comments post-commit.

Yup, that's totally fine, especially when there was a thorough pre-commit code review.

Clarification: it's totally fine to submit without waiting for me longer than a grace period of a day or so, once all comments are addressed and other reviewers have LGTM'd the patch.

However, here specifically I have a few more comments ;)

SilverGeri edited edge metadata.

remove redundant allOf statements;
refactor test's comment checking part

SilverGeri marked 4 inline comments as done.Dec 30 2016, 10:15 AM
alexfh accepted this revision.Dec 30 2016, 3:20 PM
alexfh edited edge metadata.

I've noticed a few more minor issues. Otherwise looks good.

Thank you for the new check!

clang-tidy/readability/DeleteNullPointerCheck.cpp
28–39

This can be simplified. Something like this (modulo formatting):

const auto PointerExpr = ignoringImpCasts(declRefExpr(to(decl().bind("deletedPointer"))));
const auto PointerCondition = castExpr(hasCastKind(CK_PointerToBoolean), hasSourceExpression(PointerExpr));
const auto BinaryPointerCheckCondition =
    binaryOperator(hasEitherOperand(castExpr(hasCastKind(CK_NullToPointer))),
                   hasEitherOperand(PointerExpr));

(and remove the second hasCondition).

54

The check can suggest a fix in this case as well, but it's a bit more involved. Please add a FIXME.

docs/clang-tidy/checks/readability-delete-null-pointer.rst
8

Sorry for not being clear enough: "null pointer" is not an inline code snippet, it shouldn't be enclosed in double backquotes or anything else. The "(enclosed in double backquotes)" part was meant to apply to nullptr only (since it's a keyword and should be highlighted as a code snippet).

test/clang-tidy/readability-delete-null-pointer.cpp
9

The that should not be deleted part is superfluous, IMO. You could even leave just // #1, // #2, etc.

This revision is now accepted and ready to land.Dec 30 2016, 3:20 PM
SilverGeri edited edge metadata.

reduce number hasCondition to 1;
add FIXME comment;
shorten check comments in test

SilverGeri marked 5 inline comments as done.Dec 31 2016, 4:20 AM
SilverGeri added inline comments.
docs/clang-tidy/checks/readability-delete-null-pointer.rst
8

To be honest, I don't even understand why I did what I did... :D

This revision was automatically updated to reflect the committed changes.
alexfh added a comment.Jan 2 2017, 7:42 AM

One more late comment (I should really add a check-list for new checks): this check lacks tests with macros and templates with multiple instantiations.

Incorrect handling of templates will likely not manifest in the current state of the check, it's brittle, since it relies on the error deduplication performed by clang-tidy and it can break easily (e.g. if message text will depend on the instantiation or if something changes in the way clang-tidy deduplicates messages). However, attempts to apply fixes to code resulting from macro expansions is unlikely to result in compilable code.

Via Web