Page MenuHomePhabricator

[clang-tidy] Add `delete this` bugprone check (PR38741)
AbandonedPublic

Authored by m4tx on Nov 8 2018, 7:11 AM.

Details

Summary

Add a bugprone check to clang-tidy that detects the usages of delete this. The warning is shown even when delete this is the last line in the method (as there is no guarantee that it won't be called from another method, or the pointer won't be used in any way afterwards).

https://bugs.llvm.org/show_bug.cgi?id=38741

Diff Detail

Event Timeline

m4tx created this revision.Nov 8 2018, 7:11 AM

It's best to use clang-tidy/add_new_check.py tool.
You also need tests, and a note in releasenotes.

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

docs/clang-tidy/checks/bugprone-delete-this.rst
8

Please use 80 characters limit.

Eugene.Zelenko added a project: Restricted Project.

Please also add regression test case. Is should also cover standalone function with this variable.

aaron.ballman added inline comments.Nov 8 2018, 11:05 AM
clang-tidy/bugprone/DeleteThisCheck.cpp
23

Will this catch too much? e.g.,

struct S {
  int *Foo;

  ~S() { delete this->Foo; }
};
30

Why is it suspicious? It's a valid coding construct that does get used -- looking over LLVM's source, we have a half dozen instances of this construct outside of testing code.

I think this check is likely to have an extremely high false-positive rate unless you add some heuristics to cover common usage patterns that are safe.

m4tx added a comment.Nov 19 2018, 2:35 PM

After thinking about the possible use cases (and the difficulty of implementing heuristics for them) as well as fiddling with Clang Static Analyzer it seems that this patch can be discarded as the Analyzer already handles delete this pretty well. I've posted an update to the Bugzilla ticket.

Ok, Thank you for looking into it. You can abondon the revision in
phabricator (Selection above comment field at the bottom of the page) if
you wish.

Am 19.11.18 um 23:36 schrieb Mateusz Maćkowski via Phabricator:

m4tx added a comment.

After thinking about the possible use cases (and the difficulty of implementing heuristics for them) as well as fiddling with Clang Static Analyzer it seems that this patch can be discarded as the Analyzer already handles delete this pretty well. I've posted an update to the Bugzilla ticket.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D54262

m4tx abandoned this revision.Nov 26 2018, 11:03 AM