0
votes

I am trying to write the memory area described by the unique_ptr buffer and uint32_t bufferSize into the std::vector value. When I use std::memcpy for this, I get the following warning:

‘void* memcpy(void*, const void*, size_t)’ writing to an object of type ‘class std::vector<unsigned char, std::allocator >’ with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Wclass-memaccess]

int ReadByName (std::vector<unsigned char> &value, const char handleName[], std::ostream &out, const long &port, const AmsAddr &server)
{
        uint32_t bytesRead;

        out << __FUNCTION__ << "():  ";
        
        if (this->handle == 0)
            handle = getHandleByName(out, port, server, handleName);
        if (bufferSize == 0)
            bufferSize = getSymbolSize(out, port, server, handleName);

        const auto buffer = std::unique_ptr<uint8_t>(new uint8_t[bufferSize]);

        const long status = AdsSyncReadReqEx2(  port,
                                                &server,
                                                ADSIGRP_SYM_VALBYHND,
                                                handle,
                                                bufferSize,
                                                buffer.get(),
                                                &bytesRead);
        
        if (status) {
            out << "ADS read failed with: " << std::dec << status << '\n';
            return 2;
        }
        value.clear();
        value.reserve(bufferSize);
        std::memcpy(&value, buffer.get(), bufferSize);
        return 0;
}

What would be the correct way to approach the problem?

Regards Tillman

2
value.assign(buffer.get(), buffer.get() + bufferSize);?Jarod42
Off topic but... buffer should be initialized using std::unique_ptr<uint8_t[]>(new uint8_t[bufferSize]) rather than std::unique_ptr<uint8_t>(new uint8_t[bufferSize]) -- note the extra [].G.M.
Thanks, i think this is what i was looking for @Jarod42.Tillman

2 Answers

4
votes

What would be the correct way to approach the problem?

It would be to read into a vector directly, rather than a separate array that you later copy.

In case the read operation with AdsSyncReadReqEx2() fails, value would have already been cleared.

Don't clear value, but use another vector. If operation succeeds, move-assign the intermediate vector to value which doesn't require copying of the elements.

Even better: Don't pass value into the function at all, but return the vector and use more modern error handling mechanism than a plain error code. You can use for example exceptions or something similar to the proposed std::expected.

3
votes

Two mistakes, here's the correct code

    value.clear();
    value.resize(bufferSize);
    std::memcpy(value.data(), buffer.get(), bufferSize);

First mistake, &value does not give you the data area of the vector for you to write into value.data() does that.

Second mistake, reserve does not change the size of a vector so you are writing into an empty vector. Use resize to change the size of a vector.

EDIT

Obviously what eerorika says is true. You can adapt the above code to pass a valid pointer to the vector data to AdsSyncReadReqEx2 and do away with buffer completely.

 value.resize(bufferSize);
 const long status = AdsSyncReadReqEx2(  port,
        &server,
        ADSIGRP_SYM_VALBYHND,
        handle,
        value.size(),
        value.data(),
        &bytesRead);