0
class Attr
{
public:
   Attr();
   Attr(const std::wstring& name)
   {
      ...
   }
};

class AttrDec : public Attr
{
public:
   AttrDec(Attr* attr)
      :Attr()
   {
      _attr = attr;
   }
   AttrDec(Attr*&& attr)
      :Attr()
   {
      _attr = std::move(attr);
   }

private:
   Attr* _attr;
};

class XAttr : public AttrDec
{
public:
   XAttr(const std::wstring& name)
      :AttrDec(&Attr(name)) //HERE!!!
   {}
}

at the marked position I got a warning:

nonstandard extension used: class rvalue used as lvalue.

But I have defined move constructor in class AttrDec!

How can I solve this warning?

vishal
  • 2,258
  • 1
  • 18
  • 27
rich
  • 93
  • 1
  • 1
  • 10
  • 2
    what does it even mean to move a pointer? it has no meaning. it falls back to simple copying.. lose the move semantics – David Haim Nov 18 '15 at 16:58
  • You can't use the address of a temporary. It will be destructed once the constructor is finished. – Anon Mail Nov 18 '15 at 17:00
  • 1
    It is unclear what you are trying to do. What is the expected lifetime of the data pointed to by `_attr`? In C++, you have to pay attention to data lifetime. If you don't know, your code will only work by accident. What you are seeing is the compiler saying "you took a pointer to something with a nearly zero lifetime; you probably screwed up". There are many ways to get it to *compile*, but to get it to *work* you need to fix your lifetime problem. – Yakk - Adam Nevraumont Nov 18 '15 at 17:03
  • Side note: you don't need to specify the default constructor of base class in initializer list: `AttrDec(Attr*&& attr) :Attr() {}`, you can drop the `:Attr()`. – Patryk Nov 18 '15 at 21:36
  • I just want to do a decorator. @Yakk https://en.wikipedia.org/wiki/Decorator_pattern – rich Nov 19 '15 at 09:05

4 Answers4

1

AttrDec(&Attr(name)) is trying to take the address of a temporary object. The address-of operator can only be used on lvalues.

What you could do is change XAttr to

class XAttr : public AttrDec
{
public:
   XAttr(const std::wstring& name)
      :AttrDec(new Attr(name)) // create a pointer here
   {}
};

And then get rid of the rvalue constructor in AttrDec. You would then need to add a destructor to AttrDec and delete the pointer in it and add a copy constructor to properly copy the pointer otherwise there would be a memory leak. For more on having a resource mannaging class see: What is The Rule of Three?

Community
  • 1
  • 1
NathanOliver
  • 171,901
  • 28
  • 288
  • 402
  • 1
    So your advice is to leak resources to "solve" the OP's problem? – Yakk - Adam Nevraumont Nov 18 '15 at 17:02
  • And you have to make sure that the copy and move constructor/operator are disabled or correctly implemented... – Mark B Nov 18 '15 at 17:04
  • 1
    This question is not about whether the `AttrDec` class should store an object, a raw pointer, or a smart pointer. It's about how to store `name` wrapped in an `Attr` in `AttrDec`. Let's not confuse the issues and not downvote a correct answer. – R Sahu Nov 18 '15 at 17:13
1

The right way to solve this is to store by value and then let move do its thing. Moving pointers won't move the underlying temporary value that will still go away, and you'll have a moved dangling pointer to nowhere:

class Attr
{
public:
   Attr();
   /*explicit depending on needs*/ Attr(const std::wstring& name)
   {
      ...
   }
}

class AttrDec : public Attr
{
public:
   AttrDec(Attr attr)
      :Attr()
      , attr_(attr)
   {
   }
   AttrDec(Attr&& attr)
      :Attr()
      , attr_(std::move(attr))
   {
   }

private:
   Attr attr_;
}

class XAttr : public AttrDec
{
public:
   XAttr(const std::wstring& name)
      : AttrDec(Attr(name)) //HERE!!!
   {}
}
Mark B
  • 95,107
  • 10
  • 109
  • 188
  • This will be OK as long as there are no sub-types of `Attr`. It breaks down if `Attr` has sub-types: `struct IntAttr : public Attr { ... };`. – R Sahu Nov 18 '15 at 17:06
  • It's nonobvious that the OP actually wants dynamic polymorphism. And if he does, IMO it's cleaner to make a wrapper with value semantics that holds a pointer inside. –  Nov 19 '15 at 12:15
0

Rvalues were added to the language to prevent extensive copying of objects, the creation of temporaries etc, but pointers are in essence simple ints so you are not gaining anything by using them on an int. The overhead of using rvalues will almost certainly be higher than just copying the pointer.

ventsyv
  • 3,316
  • 3
  • 27
  • 49
0

You need lifetime management.

Aasuming non-view semantics on decoration (the decorator owns the decorated), and no need for value samantics (rare if your base class is abstract), this may work:

struct Attr {
  Attr();
  Attr(const std::wstring& name)
  {}
};

struct AttrDec : Attr {
  AttrDec(std::unique_ptr<Attr>&& attr){
    _attr = std::move(attr);
  }
private:
  std::unique_ptr<Attr> _attr;
};

struct XAttr : AttrDec {
  XAttr(const std::wstring& name)
   :AttrDec(std::make_unique<Attr>(name))
  {}
};

If your std lacks make_unique (C++14), here is an implementation:

namespace notstd{
  template<class T,class...Args>
  std::unique_ptr<T> make_unique(Args&&...args){
    return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
  }
}

Which is far cleaner than a raw call to new.

The main difference is that the above makrs ownership explicit.

Variations that use shared pointers, value semantics pointers with clone support, or even view pointers can work: the last option requires XAttr provide independent storage in a second base class for the attr.

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524