Page MenuHomePhabricator

Enhance MakeSmartPtrCheck for work with class hierarchies
Needs ReviewPublic

Authored by ZaMaZaN4iK on Thu, Jan 3, 4:39 PM.

Details

Summary

In this patch I've tried to fix some issue which was reported by user. We have this code:

#include <memory>

struct Base{
    virtual ~Base() = default;
    virtual void onTest() {};
};

struct Derived : Base {
    void onTest() override {

    }
};

class A {
    std::unique_ptr<Base> base_;
public:
    A() {
        base_ = std::unique_ptr<Base>(new Derived); // Missing warning
    }
};

void foo()
{
    A a;
}

So I've added one more check for this specific case.

NB: by this change clang-tidy can report some breaking changes, e.g.:

// Types will be different
auto origin = std::unique_ptr<Base>(new Derived);
auto modified = std::make_unique<Derived>();

Suggestions are welcomed.

Diff Detail

Event Timeline

ZaMaZaN4iK created this revision.Thu, Jan 3, 4:39 PM
hokein added a subscriber: hokein.Fri, Jan 4, 12:33 AM

Thanks for the contribution.

I believe your patch is addressing the FIXME in https://github.com/llvm-mirror/clang-tools-extra/blob/master/test/clang-tidy/modernize-make-unique.cpp#L194, could you please run the test ninja check-clang-tools to make sure all check tests passed?

NB: by this change clang-tidy can report some breaking changes, e.g.:

// Types will be different
auto origin = std::unique_ptr<Base>(new Derived);
auto modified = std::make_unique<Derived>();

This new behavior seems too aggressive, it may break compilation, if there are other code statements like origin.reset(new Base()) after auto origin. I think we should be conservative about this case, but for the case like std::unique_ptr<Base> origin = std::unique_ptr<Base>(new Derived);, the new behavior is safe.

@hokein Is there any policy about the question "Which changes can clang-tidy check suggest?" . Should clang-tidy check suggest only "safe" fixes and warn only in "safe" cases?

@hokein Is there any policy about the question "Which changes can clang-tidy check suggest?" . Should clang-tidy check suggest only "safe" fixes and warn only in "safe" cases?

In general, clang-tidy can have a higher false positive rater for diagnostics than the static analyzer or the compiler proper, but check authors should strive for a reasonably low false positive rate (where "reasonable" is a bit subjective). However, fix-its should have nearly zero false positives because it's a very bad user experience otherwise ("your code is wrong, but press this button and we'll make it better; oops, that code is wrong too now, sorry!"). If you can lower the FP rate without undue burden for the check, that's generally the preferred approach.

Can anyone help me with writing a checker for such code:

auto ptr = std::unique_ptr<Base>(new Derived);

I tried a lot of different combinations of matchers, but no one didn't match this line. Without such checker there is no chance, I think, fix current smart check in right way.

JonasToth added a comment.EditedMon, Jan 7, 8:49 AM

Can anyone help me with writing a checker for such code:

auto ptr = std::unique_ptr<Base>(new Derived);

I tried a lot of different combinations of matchers, but no one didn't match this line. Without such checker there is no chance, I think, fix current smart check in right way.

Do you use clang-query to prototype your matcher?

*EDIT* Sorry, I didn't fully read the revision before, I will think about something in a moment.

Please add test-cases that demonstrate your change in behaviour. Especially your problematic cases that could be a false positive/result in wrong transformation shuold be in the tests with explicit annotation what happens/nothing bad happens.

clang-tidy/modernize/MakeSmartPtrCheck.cpp
128

I think getTypePtr() is redundant here, as getAllocatedType() seems to be a QualType. You can just use -> on a QualType to go to the underying Type

130

Comment with caution: The inheritance could be hidden behind typedefs, I think that would not be resolved with your current code. Please provide some tests to make it easier to reason about what happens.