5

I'm adapting the idea of the deffered ptr Herb Sutter talked about in cppcon 2016, to be able to manage external resources represented by an id in a safer way.

Therefore I created a non copy able and only movable class holding the id the resource should represent. Like a unique_ptr the id should become 0 if the object is moved to another object.

To my understanding you should still be allowed to use the object even after is was moved, if the called function does not have any preconditions, so to my understanding this should be valid:

int main() {

    resource src = make_resource(10);
    resource dst;
    std::cout << "src " << src.get() << std::endl;
    std::cout << "dst " << dst.get() << std::endl;

    dst = std::move(src);
    std::cout << "src " << src.get() << std::endl; // (*)
    std::cout << "dst " << dst.get() << std::endl;

    src = make_resource(40);
    std::cout << "src " << src.get() << std::endl;
    std::cout << "dst " << dst.get() << std::endl;

    return 0;
}

But clang-tidy give me this warning:

warning: 'src' used after it was moved [bugprone-use-after-move]

For the src.get() after the dst = std::move(src) (marked above).

So my questions are:

  1. Am I allowed to call src.get() after the std::move(src)
  2. May I make the assumption that src.get() returns 0 after the std::move.
  3. If 1. and 2. are valid, then is there a way to change the code so that clan-tidy knows that this is valid. And if not is there a way to change the code that it is valid?

Here is the implementation of the class:

struct resource {
    resource() = default;

    // no two objects are allowed to have the same id (prevent double free, only 0 is allowed multiple times as it represents nullptr)
    resource(const resource&) = delete;
    resource& operator=(const resource& other) = delete;

    // set the id of the object we move from back to 0 (prevent double free)
    resource(resource&& other) noexcept : id(std::exchange(other.id, 0)) {}
    resource& operator=(resource&& other) noexcept {
        id = std::exchange(other.id, 0);
        return *this;
    }

    // will free the external resource if id not 0
    ~resource() = default;

    // returns the id representing the external resource
    int get() const noexcept { return id; }

  protected:
    // only allow the make function to call the constructor with an id
    friend resource make_resource(int id);
    explicit resource(int id) : id(id) {}

  protected:
    int id = 0; // 0 = no resource referenced
};

// in the final version the id should be retrieved by from the external ip
resource make_resource(int id) { return std::move(resource(id)); }
Barry
  • 286,269
  • 29
  • 621
  • 977
t.niese
  • 39,256
  • 9
  • 74
  • 101
  • I think it's fine, but clang-tidy doesn't know about your smart pointer and its semantics, so assumes the worse. – Matthieu Brucher Jan 11 '19 at 13:50
  • Didn't you write the code? If so, you should be the best person to answer those questions. – juanchopanza Jan 11 '19 at 13:53
  • 1
    there is not much difference between a moved-from `resource` and a valid one that happens to have `id==0`, no? – 463035818_is_not_an_ai Jan 11 '19 at 13:53
  • @juanchopanza If I step through with the debugger, then I see that everything works as expected. But that does not mean that this is undefined behavior. – t.niese Jan 11 '19 at 13:54
  • clang-tidy is only warning for this pattern that *could* be bad. – Matthieu Brucher Jan 11 '19 at 13:55
  • afaik its just convention that a moved from object can be in an invalid state, if moving does not put your objects in an invalid state, then no worries – 463035818_is_not_an_ai Jan 11 '19 at 13:56
  • @user463035818 the external API represents a `nullptr` reference to nothing as `0` and all other resources have unique ids, but I don't have direct access to those resources, I can just allocate the resource and get the `id` and release it using that `id` and manipulate it using that`id`. So `0` is just like `nullptr`. – t.niese Jan 11 '19 at 13:56
  • sorry, dont completely understand. What I meant is: forget about moving for a second, then you can create a valid object with `id=0`. Now back to moving: after the move, the only thing that changed is that `id=0`. You have no way to tell the difference between the two objects, do you? – 463035818_is_not_an_ai Jan 11 '19 at 13:59
  • @juanchopanza, since when being the author of the code makes you aware of all the language behaviours? If it worked that way, there would never be any questions. Sometimes bugs we're umaware of might be spotted by an analysing tool, like clag-tidy. When unsure whether they spot correct errors, questions should be asked. The situation would be different if OP was the author of both this code and clang-tidy - then I would totally agree with you, but I suspect that's not the case. – Fureeish Jan 11 '19 at 13:59
  • 1
    @user463035818 As far as I've understood, no valid object managed by this class can ever have id = 0, as guaranteed by the external API. – Baum mit Augen Jan 11 '19 at 14:02

2 Answers2

8
  1. Am I allowed to call src.get() after the std::move(src)

If we remain agnostic about the type of src, then we don't know. Potentially not. There are cases where calling a member function after move would be undefined. For example, calling the indirection operator of a smart pointer.

Given the definition of decltype(src) and its member functions that you've shown, we know: Yes, you're allowed to.

  1. May I make the assumption that src.get() returns 0 after the std::move.

If we remain agnostic about the type of src, we know nothing about what src.get() does. More specifically, we don't know its preconditions.

Given the definition of decltype(src) and its member functions that you've shown: Yes, we can make the assumption.

  1. If 1. and 2. are valid, then is there a way to change the code so that clan-tidy knows that this is valid. And if not is there a way to change the code that it is valid?

Clang-tidy assumes that "not being in a moved from state" is a precondition of all member functions (other than assignment), and under that assumption, is warning that such supposed precondition is being violated. As such, it is trying to enforce a convention to always assume such precodition even though you happen to know that it doesn't exist for your class.

You can remove the call to src.get() between the move, and the re-assignment of src. One operation on a moved from variable that clang-tidy doesn't complain about is re-assignment and after that assignment, the state of the object should (conventionally) be well defined and calling other member functions is assumed to be fine (of course, you can have other pre-conditions which must be met, but clang-tidy probably doesn't know of them). Although technically it would be possible to define a type for which even the assignment after move is not well defined, but such type would be quite unconventional and unsafe.


To conclude, you can call src.get() after move (even before reassignment) for this particular class, but then you won't be following the convention that clang-tidy is trying to enforce.

eerorika
  • 232,697
  • 12
  • 197
  • 326
  • That makes sense I was so focused on whether my implementation was right that I forgot the fact that you naturally don't access the object after a move even if it was right. So in normal use-case the clang-tidy warning would not appear, because you should never call the `.get()` directly after a move, and that's what clang-tidy is warning me about. But I could call `.get()` later somewhere else, to check if my class is still referencing the external resource or if the "ownership" was moved away. – t.niese Jan 11 '19 at 14:09
  • @t.niese That's the thing, sometimes you *would* call `.get()`, because it depends entirely on the type of the object being moved from. That is why I made my comment about authorship of the code. clang-tidy has to make assumptions in order to function, and sometimes these will be wrong. – juanchopanza Jan 11 '19 at 17:12
7

cppreference.com has this text:

Unless otherwise specified, all standard library objects that have been moved from are placed in a valid but unspecified state. That is, only the functions without preconditions, such as the assignment operator, can be safely used on the object after it was moved from:

So, informally, the C++ convention is that the moved-from object will be valid but useless, which is why clang-tidy is suggesting it is suspicious to be using it.

For your implementation, you have provided 'more' than convention – so your code is not wrong, just unconventional.

Baum mit Augen
  • 49,044
  • 25
  • 144
  • 182
Peter Hull
  • 6,683
  • 4
  • 39
  • 48
  • To be fair, the quote says nothing about non-standard-library objects - I realise that you are quite deliberately calling this an informal convention because you realise that you're extending the scope of the quote, and that's not unreasonable. But I'm not sure whether we can even go that far, I guess is what I'm trying to say. Meh – Lightness Races in Orbit Jan 11 '19 at 15:43
  • "valid but useless" is a bit strong for an object whose interface with no pre-conditions you can call. An the implementation doesn't provide more than the convention. It is just that `pre()` has no pre-conditions. – juanchopanza Jan 11 '19 at 17:15
  • I meant `get()`, not `pre()`. – juanchopanza Jan 11 '19 at 17:21
  • 2
    @LightnessRacesinOrbit 'informal convention' may not be the right term but what I mean is - IMO it is unwise to design things in way that would be surprising to someone who understood the C++ standard library, even though C++-the-language can't enforce that. To take a silly example, nothing but convention stops you from making a container class where the method `begin()` deletes the contents. – Peter Hull Jan 11 '19 at 19:36
  • FWIW I don't particularly disagree. And your last comment is a better way to put it IMO – Lightness Races in Orbit Jan 12 '19 at 16:48
  • Note that "Valid" only means that the destructor will work correctly. Consider unique_ptr. After using std::move to initialize or assign a unique_ptr, the other pointer becomes NULL. Its state is okay to destruct, but not okay to dereference. – Dwedit May 27 '22 at 20:17