[C++] Compiler remove for-loop in putString methods

[ksergey]

Hello devs!

I have a message:

<type name="String64" primitiveType="char" length="64" semanticType="String"/>
...
    <message id="3" name="MarketDataResponse">
...
        <field name="Text" id="7" type="String64"/>
    </message>

The sbe-tool generate c++ code:

    MarketDataResponse &putText(std::string_view str)
    {
        const size_t srcLength = str.length();
        if (srcLength > 64)
        {
             throw std::runtime_error("string too large for putText [E106]");
        }

        size_t length = srcLength < 64 ? srcLength : 64;
        std::memcpy(m_buffer + m_offset + 54, str.data(), length);
        for (size_t start = srcLength; start < length; ++start)
        {
            m_buffer[m_offset + 54 + start] = 0;
        }

        return *this;
    }

The problem is compiler (gcc 7 in my case) removes for-loop in case of flag -O2 set.

Is it possible to fix the issue inside sbe-tool?

Thanks

[ksergey]

https://godbolt.org/z/Ih_3Ye
Play with flags -O2, -O1

[tmontgomery]

This sounds more like it is the result of the compiler finding out how it is being used and removing it. So, it is probably more the usage that can control that than the actual generated code.

[ksergey]

Looks like the problem in generated code, because srcLength == length. xD

sbe-all-1.12.2.jar used for generation.

[ksergey]

Also one of string modify methods return copy of *this instead of reference

[mjpt777]

@ksergey which variant returns a copy?

[tmontgomery]

If str.length() is 64, then you don't want the padding. You only want it when str.length() < 64.... so, you don't want the for loop..... unless you want to have a NULL terminated value.... which is NOT what this is trying to do.

[ksergey]

@mjpt777 looks like the issue solved in 1.12.4

@tmontgomery
Schema test.xml:

<messageSchema package="sbe" id="0" version="0" byteOrder="littleEndian">
    <types>
        <type name="String20" primitiveType="char" length="20" semanticType="String"/>
        <composite name="messageHeader" description="Template ID and length of message root">
            <type name="blockLength" primitiveType="uint16"/>
            <type name="templateId" primitiveType="uint16"/>
            <type name="schemaId" primitiveType="uint16"/>
            <type name="version" primitiveType="uint16"/>
        </composite>
    </types>

    <message name="Message" id="1">
        <field name="Text" id="1" type="String20"/>
    </message>
</messageSchema>

Generated code (java -Dsbe.target.language=CPP -Dsbe.output.dir=. -jar sbe-all-1.12.4-all.jar test.xml):

...
    #if __cplusplus >= 201703L                                                                                                              [9/930]
    Message &putText(const std::string_view str)
    {
        const size_t srcLength = str.length();
        if (srcLength > 20)
        {
            throw std::runtime_error("string too large for putText [E106]");                                                                      
        }

        size_t length = srcLength < 20 ? srcLength : 20;
        std::memcpy(m_buffer + m_offset + 0, str.data(), length);
        for (size_t start = srcLength; start < 20; ++start)
        {
            m_buffer[m_offset + 0 + start] = 0;
        }

        return *this;
    }
    #else
    Message &putText(const std::string& str)
    {
        const size_t srcLength = str.length();
        if (srcLength > 20)
        {
            throw std::runtime_error("string too large for putText [E106]");
        }

        size_t length = srcLength < 20 ? srcLength : 20;
        std::memcpy(m_buffer + m_offset + 0, str.c_str(), length);
        for (size_t start = srcLength; start < 20; ++start)
        {
            m_buffer[m_offset + 0 + start] = 0;
        }

        return *this;
    }
    #endif
...

Looks like this issue resolved too in sbe-all-1.12.4.
Any way length always equal to srcLength:
if srcLength == 20 length = srcLength < 20 ? srcLength : 20 = 20 < 20 ? 20 : 20 = 20
if srcLength == i.e. 5 length = srcLength < 20 ? srcLength : 20 = 5 < 20 ? 5 : 20 = 5

Feel free to close the issue.

[tmontgomery]

AH. I see. I was looking at the current code and not the version you mentioned. Glad it is taken care of.