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

alexfh added a comment. 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

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL290784: [clang-tidy] Add delete null pointer check. (authored by xazax). Changed prior to commit: https://reviews.llvm.org/D21298?vs=82760=82761#toc Repository: rL LLVM

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

SilverGeri marked 5 inline comments as done. SilverGeri added inline comments. Comment at: docs/clang-tidy/checks/readability-delete-null-pointer.rst:7 +Checks the 'if' statements where a pointer's existence is checked and then deletes the pointer. +The check is unnecessary as

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

SilverGeri updated this revision to Diff 82760. SilverGeri added a comment. reduce number `hasCondition` to 1; add FIXME comment; shorten check comments in test https://reviews.llvm.org/D21298 Files: clang-tidy/readability/CMakeLists.txt

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

alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. I've noticed a few more minor issues. Otherwise looks good. Thank you for the new check! Comment at: clang-tidy/readability/DeleteNullPointerCheck.cpp:27-38 + const auto

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

SilverGeri updated this revision to Diff 82732. SilverGeri added a comment. remove redundant `allOf` statements; refactor test's comment checking part https://reviews.llvm.org/D21298 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeleteNullPointerCheck.cpp

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

alexfh added a comment. In https://reviews.llvm.org/D21298#632265, @alexfh wrote: > In https://reviews.llvm.org/D21298#632235, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D21298#632154, @xazax.hun wrote: > > > > > Small ping, is this ready to be committed? > > > > > > If @alexfh

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

alexfh added a comment. In https://reviews.llvm.org/D21298#632235, @aaron.ballman wrote: > In https://reviews.llvm.org/D21298#632154, @xazax.hun wrote: > > > 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

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

alexfh requested changes to this revision. alexfh added inline comments. This revision now requires changes to proceed. Comment at: clang-tidy/readability/DeleteNullPointerCheck.cpp:29 + const auto BinaryPointerCheckCondition = binaryOperator( +

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

aaron.ballman added a comment. In https://reviews.llvm.org/D21298#632154, @xazax.hun wrote: > 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.

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

xazax.hun added a comment. Small ping, is this ready to be committed? https://reviews.llvm.org/D21298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

SilverGeri updated this revision to Diff 82401. SilverGeri added a comment. remove brackets https://reviews.llvm.org/D21298 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeleteNullPointerCheck.cpp clang-tidy/readability/DeleteNullPointerCheck.h

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

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM, with one nit. You should wait for @alexfh to sign off before committing though, since he requested changes. Comment at: clang-tidy/readability/DeleteNullPointerCheck.cpp:53 + "'if' statement is

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

SilverGeri updated this revision to Diff 81721. SilverGeri added a comment. removing redundant `allOf` from `ifStmt` https://reviews.llvm.org/D21298 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeleteNullPointerCheck.cpp

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

Prazek added inline comments. Comment at: clang-tidy/readability/DeleteNullPointerCheck.cpp:33 + Finder->addMatcher( + ifStmt(allOf(hasCondition( + anyOf(PointerCondition, BinaryPointerCheckCondition)), I think allOf matcher is

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

SilverGeri updated this revision to Diff 81458. https://reviews.llvm.org/D21298 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeleteNullPointerCheck.cpp clang-tidy/readability/DeleteNullPointerCheck.h clang-tidy/readability/ReadabilityTidyModule.cpp

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

SilverGeri updated this revision to Diff 81452. SilverGeri added a comment. remove unused string using early exit in condition shorten check-message lines add check-fisex to 'else' part https://reviews.llvm.org/D21298 Files: clang-tidy/readability/CMakeLists.txt

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

alexfh requested changes to this revision. alexfh added inline comments. This revision now requires changes to proceed. Comment at: clang-tidy/readability/DeleteNullPointerCheck.cpp:52 + + auto D = diag( + IfWithDelete->getLocStart(), Rename `D` to `Diag`,

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

hokein accepted this revision. hokein added a reviewer: hokein. hokein added a comment. It looks good to me, but you'd better wait for the approval from @aaron.ballman. Comment at: clang-tidy/readability/DeleteNullPointerCheck.cpp:55 + "'if' statement is unnecessary;

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

SilverGeri updated this revision to Diff 79338. SilverGeri added a comment. Herald added a subscriber: JDevlieghere. only warn, not fix when the 'if' statement has 'else' clause keeping comments https://reviews.llvm.org/D21298 Files: clang-tidy/readability/CMakeLists.txt

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

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

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

SilverGeri 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]

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

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

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

hokein added inline comments. Comment at: test/clang-tidy/readability-delete-null-pointer.cpp:3 + +#include + We don't rely on implementations of standard headers in lit test. You should fake the function/class that you need in this test.

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

SilverGeri updated this revision to Diff 77972. SilverGeri added a comment. update tests with "CHECK-FIXES-NOT" parts https://reviews.llvm.org/D21298 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeleteNullPointerCheck.cpp

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

SilverGeri updated this revision to Diff 77784. SilverGeri added a comment. Herald added a subscriber: modocache. move checker to readability module add missing description to header file https://reviews.llvm.org/D21298 Files: clang-tidy/readability/CMakeLists.txt

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

Prazek added inline comments. Comment at: test/clang-tidy/misc-delete-null-pointer.cpp:11 + } + // CHECK-FIXES: delete p; + int *p3 = new int[3]; Is there check-fixes-not? This seems to be required here, because even if the fixit won't happen here, the test

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

aaron.ballman added inline comments. Comment at: clang-tidy/misc/DeleteNullPointerCheck.cpp:54 + diag(IfWithDelete->getLocStart(), + "if statement is unnecessary (deleting null pointer has no effect)"); + std::string ReplacementText = Lexer::getSourceText(

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

SilverGeri updated this revision to Diff 77015. SilverGeri added a comment. checks `if (p != 0)` conditions, too https://reviews.llvm.org/D21298 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/DeleteNullPointerCheck.cpp clang-tidy/misc/DeleteNullPointerCheck.h

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

alexfh added a comment. I guess, "readability" will be a better category for this check. https://reviews.llvm.org/D21298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

SilverGeri updated this revision to Diff 76893. https://reviews.llvm.org/D21298 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/DeleteNullPointerCheck.cpp clang-tidy/misc/DeleteNullPointerCheck.h clang-tidy/misc/MiscTidyModule.cpp docs/clang-tidy/checks/list.rst

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

SilverGeri removed rL LLVM as the repository for this revision. SilverGeri updated this revision to Diff 76803. Herald added a subscriber: mgorny. https://reviews.llvm.org/D21298 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/DeleteNullPointerCheck.cpp

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

Eugene.Zelenko added inline comments. Comment at: docs/clang-tidy/checks/misc-delete-null-pointer.rst:10 +.. code:: c++ +int *p; + if (p) Please indent variable declaration. https://reviews.llvm.org/D21298 ___

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

aaron.ballman added inline comments. Comment at: clang-tidy/misc/DeleteNullCheck.cpp:23 @@ +22,3 @@ + const auto HasDeleteExpr = + cxxDeleteExpr(hasDescendant(declRefExpr().bind("pointerToDelete"))) + .bind("deleteExpr"); etienneb wrote: > The use

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

etienneb added a comment. Some comments after a quick look to the code. Comment at: clang-tidy/misc/DeleteNullCheck.cpp:22 @@ +21,3 @@ +void DeleteNullCheck::registerMatchers(MatchFinder *Finder) { + const auto HasDeleteExpr = +

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

Eugene.Zelenko added a comment. 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. Repository: rL LLVM

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

Eugene.Zelenko added a subscriber: Eugene.Zelenko. Eugene.Zelenko added a comment. Please mention this check in docs/ReleaseNotes.rst (in alphabetical order). I think misc-delete-null-pointer will be better name. Repository: rL LLVM http://reviews.llvm.org/D21298