bugprone check for delete this

Quuxplusone commented 6 years ago


Bugzilla Link	PR38741
Status	NEW
Importance	P enhancement
Reported by	Eugene Zelenko (eugene.zelenko@gmail.com)
Reported on	2018-08-28 10:26:46 -0700
Last modified on	2018-11-19 14:36:52 -0800
Version	unspecified
Hardware	All All
CC	alexfh@google.com, development@jonas-toth.eu, djasper@google.com, klimek@google.com, mateusz@mackowski.org
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

It'll be great to bugprone check for delete this in class methods.

Quuxplusone commented 6 years ago

I added the beginner tag as this sounds not too complicated.

If someone tries to implement, don't hesitate to ask (here, IRC or mailing list) if you are stuck!

Quuxplusone commented 5 years ago

It seems that Clang Static Analyzer already handles this pretty well. For a
class like so:

    class Foo {
      int x;

    public:
      void cleanup() { delete this; }

      void test1() {
        cleanup();
        test2();
      }

      void test2() {}
    };

clang-tidy produces:

    1 warning generated.
    /tmp/tidytest/foo.cpp:13:5: warning: Use of memory after it is freed [clang-analyzer-cplusplus.NewDelete]
        test2();
        ^
    [...]
    /tmp/tidytest/foo.cpp:9:20: note: Memory is released
      void cleanup() { delete this; }
                       ^

Because of that, I believe we do not need to add another check, especially
since without the usage of static analysis, we would need to implement some
kind of heuristics to cover common use cases of `delete this`. Therefore, I
think this ticket can be closed.

Quuxplusone commented 5 years ago

I'm not sure about present situation, but in past Static Analyzer worked only in single translation unit, so not all problematic cases could be detected.

Quuxplusone / LLVMBugzillaTest

bugprone check for delete this #37714