Char array in a struct - not renewing?

Question

I have a for-loop and i'm creating a new instance of a struct on the stack each time. This struct just contains 2 variables - 2 char arrays of 64 bytes.

The code is below:

        for (std::map<std::string, std::string>::iterator iter = m_mDevices.begin(); iter != m_mDevices.end(); ++iter)
        {
            Structs::SDeviceDetails sRecord;
            if (false == GenerateDeviceCacheRecord(iter->first, iter->second, sRecord)) // could just pass iter in?
            {
                // Failed to create cache record
                return false;
            }   
        }

The really strange thing i am seeing in the debugger, is everytime i loop round, i am seeing the same value in sRecord's buffers. i.e. sRecord.m_strUsername and sRecord.m_strPassword is getting "written over" as opposed to being a newly created struct.

If sRecord.m_strUsername was "abc" on the first loop round, then after the GenerateDeviceCacheRecord function (which just modifies sRecord), sRecord.m_strUsername might be "HIc", where c is the character off the first loop! I'm obviously expecting "abc" and "HI", not "abc" and "HIc". Does anyone know what might be going on here?

Thanks

Extra code:

namespace Constants
{
    static const int64 MAX_HOSTNAME_BUFFER              = 64;
    static const int64 MAX_ILA_BUFFER                   = 64;
};

    struct SDeviceRecordDetails
    {
        char        m_strHostname[Constants::MAX_HOSTNAME_BUFFER];
        char        m_strILA[Constants::MAX_ILA_BUFFER];
    };  

bool GenerateDeviceCacheRecord(std::string strHostname, std::string strILA, Structs::SDeviceRecordDetails& sRecord)
{
    // Convert strings to char arrays to store in the authentication cache manager records
    if (strHostname.length() > Constants::MAX_HOSTNAME_BUFFER)
        return false;
    if (strILA.length() > Constants::MAX_ILA_BUFFER)
        return false;

    std::copy(strHostname.begin(), strHostname.end(), sRecord.m_strHostname);
    std::copy(strILA.begin(), strILA.end(), sRecord.m_strILA);
    return true;
}

    //! @brief Devices retrieved from XML file
    std::map<std::string, std::string> m_mDevicesAuthenticated;

Answer 1

So. I appreciate that you tried to get closer to a better question. So I'm going to take some next steps with you.

What you posted wasn't really a mcve.

Here's a mcve for your problem:

#include <iostream>
#include <cstdint>
#include <map>
#include <string>
#include <algorithm>

namespace Constants
{
    static const int64_t MAX_HOSTNAME_BUFFER              = 64;
    static const int64_t MAX_ILA_BUFFER                   = 64;
};

struct SDeviceRecordDetails
{
    char m_strHostname[Constants::MAX_HOSTNAME_BUFFER];
    char m_strILA[Constants::MAX_ILA_BUFFER];
};  

bool GenerateDeviceCacheRecord(std::string strHostname, std::string strILA, SDeviceRecordDetails& sRecord)
{
    // Convert strings to char arrays to store in the authentication cache manager records
    if (strHostname.length() > Constants::MAX_HOSTNAME_BUFFER)
        return false;
    if (strILA.length() > Constants::MAX_ILA_BUFFER)
        return false;

    std::copy(strHostname.begin(), strHostname.end(), sRecord.m_strHostname);
    std::copy(strILA.begin(), strILA.end(), sRecord.m_strILA);
    return true;
}

std::map<std::string, std::string> m_mDevices;

int main() {
    m_mDevices["hello"] = "foo";
    m_mDevices["buzz"] = "bear";

    for (std::map<std::string, std::string>::iterator iter = m_mDevices.begin(); iter != m_mDevices.end(); ++iter) {
        SDeviceRecordDetails sRecord;
        const bool result = GenerateDeviceCacheRecord(iter->first, iter->second, sRecord);

        if (result == false)
            std::cout << "Failed\n";
        else
            std::cout << sRecord.m_strHostname << " " << sRecord.m_strILA << "\n";
    }
}

Things to note:

1. I can take this as is (instead of two code blocks in your question) and throw it at a compiler.
2. I included the proper #include lines.
3. There were namespaces in your type names that weren't represented in your code.
4. m_mDevicesAuthenticated != m_mDevices.
5. You didn't include anything that actually had any output.
6. What is actually in m_mDevices? This is really important to include!
7. Among other small corrections I had to apply to the code to get it to build.

What did this code do?

This code almost produces the correct output. It has an error, in that the strings that are written to sRecord are not null terminated.

Because of how compilers generate code, and that you don't explicitly clear sRecord each loop, it's likely that this is the root cause of your problem.

Let's fix that:

Instead of:

std::copy(strHostname.begin(), strHostname.end(), sRecord.m_strHostname);
std::copy(strILA.begin(), strILA.end(), sRecord.m_strILA);

Let'd do:

snprintf(sRecord.m_strHostname, Constants::MAX_HOSTNAME_BUFFER, "%s", strHostname.c_str());
snprintf(sRecord.m_strILA, Constants::MAX_ILA_BUFFER, "%s", strILA.c_str());

Or perhaps you are concerned about what sRecord starts each loop with:

In this case, sRecord is not initialized at the beginning of each loop. The compiler is free to have junk data in the struct for optimization purposes.

It happens that most compilers will place each iteration of the struct in that exact same spot in memory. This means that the junk data in the struct could be the data from the previous iteration. Or some other junk depending on how the compiler optimizations function.

You could fix this by initializing the struct to contain explicit data:

SDeviceRecordDetails sRecord = {};

What does all of this look like:

The finished code, with all the bug fixes looks like:

#include <iostream>
#include <cstdint>
#include <map>
#include <string>
#include <algorithm>

namespace Constants
{
    static const int64_t MAX_HOSTNAME_BUFFER              = 64;
    static const int64_t MAX_ILA_BUFFER                   = 64;
};

struct SDeviceRecordDetails
{
    char m_strHostname[Constants::MAX_HOSTNAME_BUFFER];
    char m_strILA[Constants::MAX_ILA_BUFFER];
};  

bool GenerateDeviceCacheRecord(std::string strHostname, std::string strILA, SDeviceRecordDetails& sRecord)
{
    // Convert strings to char arrays to store in the authentication cache manager records
    if (strHostname.length() > Constants::MAX_HOSTNAME_BUFFER)
        return false;
    if (strILA.length() > Constants::MAX_ILA_BUFFER)
        return false;

    snprintf(sRecord.m_strHostname, Constants::MAX_HOSTNAME_BUFFER, "%s", strHostname.c_str());
    snprintf(sRecord.m_strILA, Constants::MAX_ILA_BUFFER, "%s", strILA.c_str());
    return true;
}

std::map<std::string, std::string> m_mDevices;

int main() {
    m_mDevices["hello"] = "foo";
    m_mDevices["buzz"] = "bear";
    m_mDevices["zed"] = "zoo";

    for (std::map<std::string, std::string>::iterator iter = m_mDevices.begin(); iter != m_mDevices.end(); ++iter) {
        SDeviceRecordDetails sRecord = {};
        const bool result = GenerateDeviceCacheRecord(iter->first, iter->second, sRecord);

        if (result == false)
            std::cout << "Failed\n";
        else
            std::cout << sRecord.m_strHostname << " " << sRecord.m_strILA << "\n";
    }
}

And outputs:

buzz bear
hello foo
zed zoo

Which looks correct to my eyes.

Answer 2

I don't see any initialisation here. You're seeing whatever happened to be at that place in memory before, which for you, today, happens to be the previous contents of those data members.