-1
class CTextureBuffer {
    public:
        int m_width;
        int m_height;
        void* m_data;

    CTextureBuffer (int width = 0, int height = 0, void * data = nullptr) 
        : m_width (width), m_height (height), m_data (data) {}

    CTextureBuffer (CTextureBuffer& other) 
        : m_width (other.m_width), m_height (other.m_height), m_data (other.m_data) {}

    CTextureBuffer operator= (CTextureBuffer other) {
        m_width = other.m_width;
        m_height = other.m_height;
        m_data = other.m_data;
        return *this;
    }
};


void InitTexBuf (SDL_Surface* image) {
    CTextureBuffer texBuf;
    texBuf = CTextureBuffer (image->w, image->h, image->pixels);
}

Error C2679 binary '=': no operator found which takes a right-hand operand
of type 'CTextureBuffer' (or there is no acceptable conversion)

SDL_Surface::w, ::h are ints. SDL_Surface::pixels is void *. Just so nobody complains about my not explaining the parameters.

I am clueless how I would need to write a proper copy constructor here. For me it looks like there's everything in CTextureBuffer the compiler needs.

Btw, what I am actually doing is this (wip):

// read a bunch of textures for a cubemap
// omitting a filename means "reuse the previous texture for the current cubemap face"
// first filename must not be empty 
bool CTexture::Load (CArray<CString>& fileNames, bool flipVertically) {
    // load texture from file
    m_fileNames = fileNames;
    CTextureBuffer texBuf;
    for (auto const& fileName : fileNames) {
        if (fileName->Length ()) {
            SDL_Surface * image = IMG_Load ((char*) (*fileName));
            if (!image) {
                fprintf (stderr, "Couldn't find '%s'\n", (char*) (*fileName));
                return false;
            }
        texBuf = CTextureBuffer (image->w, image->h, image->pixels);
        }
        m_buffers.Append (texBuf);
    }
    return true;
}
Razzupaltuff
  • 2,250
  • 2
  • 21
  • 37
  • Aside from the answers correctly stating they should be const. Are you sure you want to copy the data*? Who own's that data? So either model data* as a std::unique_ptr or a std::shared_ptr to make it clear and then adjust your copy constructor accordingly e.g. copy the whole data if needed or copy the shared_ptr. Or indeed leave it a non-owning pointer but comment on it. – Pepijn Kramer Sep 30 '21 at 11:17
  • see https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three – 463035818_is_not_an_ai Sep 30 '21 at 11:18
  • if your class is happy with shallow copying m_data, as it seems from your code, and you don't own m_data. you can simply omit both assignement and copy constructor. The compiler provided one are just fine. But, if this is not your intention, you need to think carefully and provide a destructor as well and following the rule of three. – Alessandro Teruzzi Sep 30 '21 at 11:20
  • @P.Kramer: The real code is a little more complicated. I read a bunch of textures to create a cubemap. You can omit textures for cubemap sides, in which case the texture last read will be used for the current cubemap side. So I initialize a texBuf and reuse it when I don't have a texture. Actually this should be quite intuitive, and C++20 is totally counter-intuitive. I have added that code to my question. – Razzupaltuff Sep 30 '21 at 11:31
  • The error message is not about a copy constructor. It is about an assignment operator. They are different things. In any event, the canonical way of specifying both copy constructors and assignment operators is that they accept a `const` reference - your code has a copy constructor and assignment operator accepting a non-`const` reference (which means that `CTexture copy_of_object(object)` (creating `copy_of_object` as a copy of `object`, where both have type `CTexture`) or assignment `x = object` can potentially change the state of `object` - which is often not a good idea. – Peter Sep 30 '21 at 12:32

1 Answers1

2

Your copy constructor should accept a const&

CTextureBuffer (CTextureBuffer& other)        // wrong
CTextureBuffer (CTextureBuffer const& other)  // right

same for your copy assignment operator, and it should return a reference

CTextureBuffer operator= (CTextureBuffer other)          // wrong
CTextureBuffer& operator= (CTextureBuffer const& other)  // right

also to obey the rule of 5 you would also define your move constructor and move assigment operator

CTextureBuffer(CTextureBuffer&& other) = default;            // move construct
CTextureBuffer& operator=(CTextureBuffer&& other) = default; // move assignment
Cory Kramer
  • 114,268
  • 16
  • 167
  • 218
  • Is that really _"wrong"_? C++ standards allow copy constructors with non-const lvalue reference parameter, as well as copy assignment operator with value parameter. – Daniel Langr Sep 30 '21 at 11:19
  • Well, it solved my problem. – Razzupaltuff Sep 30 '21 at 11:20
  • 2
    @DanielLangr It is correct C++ it is wrong in intent. Those operators should never modify the original content – Pepijn Kramer Sep 30 '21 at 11:20
  • I would say it is at least non-idiomatic. Passing a non-const reference is bug prone in case you mutate the source object, and passing by value has the opportunity to produce extra copies. – Cory Kramer Sep 30 '21 at 11:21
  • Hi Cory, thanks. To be honest, I am working my way into modern C++, and I haven't really understood the stuff around "move". I haven't been doing C++ for years, and C++20 looks totally bloated with infrastructure and control statements that make using it less and less straight forward and more and more of a pain in the neck. – Razzupaltuff Sep 30 '21 at 11:22
  • 1
    @Razzupaltuff Understood. IMO a good understanding of C++11 is a great start towards "modern C++", especially [move semantics](https://stackoverflow.com/questions/3106110/what-is-move-semantics). While there are certainly useful additions in C++14/17/20/+ I think C++11 was a big change in modernizing the language. – Cory Kramer Sep 30 '21 at 11:24
  • Thanks. Btw, adding the move assignment causes an "ambiguous operator=" error (VS 2019 community, C++20 enabled). – Razzupaltuff Sep 30 '21 at 11:26