Skip to content

Commit

Permalink
[C++] Fix field precedence check issue #1031.
Browse files Browse the repository at this point in the history 
There were a couple of problems here:

1. "wrapping constructors" did not initialize the codec state (for
precedence checks) correctly, and
2. the methods returning a JSON encoded string would attempt to
transition twice.

These problems are now resolved.

I have also made a technically breaking change. I have removed an
unnecessary constructor that took in the codec state. I think it is
highly unlikely that anyone was using this constructor; therefore,
it should be okay to make the change.
  • Loading branch information
@ZachBray
ZachBray committed Dec 11, 2024
1 parent daf0283 commit 859d542
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 52 deletions.
79 changes: 27 additions & 52 deletions sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/cpp/CppGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1201,7 +1201,7 @@ private void generateVarData(
lengthCppType,
accessOrderListenerCall);

generateJsonEscapedStringGetter(sb, token, indent, propertyName, accessOrderListenerCall);
generateJsonEscapedStringGetter(sb, token, indent, propertyName);

new Formatter(sb).format("\n" +
indent + " #if __cplusplus >= 201703L\n" +
Expand Down Expand Up @@ -2307,7 +2307,7 @@ private void generateArrayProperty(
generateStringNotPresentCondition(propertyToken.version(), indent),
accessOrderListenerCall);

generateJsonEscapedStringGetter(sb, encodingToken, indent, propertyName, accessOrderListenerCall);
generateJsonEscapedStringGetter(sb, encodingToken, indent, propertyName);

new Formatter(sb).format("\n" +
indent + " #if __cplusplus >= 201703L\n" +
Expand Down Expand Up @@ -2381,14 +2381,12 @@ private void generateJsonEscapedStringGetter(
final StringBuilder sb,
final Token token,
final String indent,
final String propertyName,
final CharSequence accessOrderListenerCall)
final String propertyName)
{
new Formatter(sb).format("\n" +
indent + " std::string get%1$sAsJsonEscapedString()\n" +
indent + " {\n" +
"%2$s" +
"%3$s" +
indent + " std::ostringstream oss;\n" +
indent + " std::string s = get%1$sAsString();\n\n" +
indent + " for (const auto c : s)\n" +
Expand Down Expand Up @@ -2417,8 +2415,7 @@ private void generateJsonEscapedStringGetter(
indent + " return oss.str();\n" +
indent + " }\n",
toUpperFirstChar(propertyName),
generateStringNotPresentCondition(token.version(), indent),
accessOrderListenerCall);
generateStringNotPresentCondition(token.version(), indent));
}

private void generateConstPropertyMethods(
Expand Down Expand Up @@ -2505,7 +2502,7 @@ private void generateConstPropertyMethods(
values,
constantValue.length);

generateJsonEscapedStringGetter(sb, token, indent, propertyName, "");
generateJsonEscapedStringGetter(sb, token, indent, propertyName);
}

private CharSequence generateFixedFlyweightCode(final String className, final int size)
Expand Down Expand Up @@ -2654,34 +2651,13 @@ private static String generateHiddenCopyConstructor(final String indent, final S
return DISABLE_IMPLICIT_COPYING ? ctorAndCopyAssignmentDeletion : "";
}

private static CharSequence generateConstructorsAndOperators(
private CharSequence generateConstructorsAndOperators(
final String className,
final FieldPrecedenceModel fieldPrecedenceModel)
{
final String constructorWithCodecState = null == fieldPrecedenceModel ? "" : String.format(
" %1$s(\n" +
" char *buffer,\n" +
" const std::uint64_t offset,\n" +
" const std::uint64_t bufferLength,\n" +
" const std::uint64_t actingBlockLength,\n" +
" const std::uint64_t actingVersion,\n" +
" CodecState codecState) :\n" +
" m_buffer(buffer),\n" +
" m_bufferLength(bufferLength),\n" +
" m_offset(offset),\n" +
" m_position(sbeCheckPosition(offset + actingBlockLength)),\n" +
" m_actingBlockLength(actingBlockLength),\n" +
" m_actingVersion(actingVersion),\n" +
" m_codecState(codecState)\n" +
" {\n" +
" }\n\n",
className);

return String.format(
" %1$s() = default;\n\n" +

"%2$s" +

" %1$s(\n" +
" char *buffer,\n" +
" const std::uint64_t offset,\n" +
Expand All @@ -2695,6 +2671,7 @@ private static CharSequence generateConstructorsAndOperators(
" m_actingBlockLength(actingBlockLength),\n" +
" m_actingVersion(actingVersion)\n" +
" {\n" +
"%2$s" +
" }\n\n" +

" %1$s(char *buffer, const std::uint64_t bufferLength) :\n" +
Expand All @@ -2711,7 +2688,7 @@ private static CharSequence generateConstructorsAndOperators(
" {\n" +
" }\n\n",
className,
constructorWithCodecState);
generateCodecStateTransitionForWrapping(fieldPrecedenceModel));
}

private CharSequence generateMessageFlyweightCode(
Expand All @@ -2728,26 +2705,25 @@ private CharSequence generateMessageFlyweightCode(
final String semanticVersion = ir.semanticVersion() == null ? "" : ir.semanticVersion();


final String codecStateArgument = null == fieldPrecedenceModel ? "" : ", m_codecState";

return String.format(
"private:\n" +
"%14$s" +
"%15$s" +
"%16$s" +
" char *m_buffer = nullptr;\n" +
" std::uint64_t m_bufferLength = 0;\n" +
" std::uint64_t m_offset = 0;\n" +
" std::uint64_t m_position = 0;\n" +
" std::uint64_t m_actingBlockLength = 0;\n" +
" std::uint64_t m_actingVersion = 0;\n" +
"%17$s" +
"%16$s" +

" inline std::uint64_t *sbePositionPtr() SBE_NOEXCEPT\n" +
" {\n" +
" return &m_position;\n" +
" }\n\n" +

"%22$s" +
"%19$s" +
"%21$s" +

"public:\n" +
" static constexpr %1$s SBE_BLOCK_LENGTH = %2$s;\n" +
Expand Down Expand Up @@ -2775,7 +2751,7 @@ private CharSequence generateMessageFlyweightCode(

" using messageHeader = %12$s;\n\n" +

"%18$s" +
"%17$s" +
"%11$s" +

" SBE_NODISCARD static SBE_CONSTEXPR %1$s sbeBlockLength() SBE_NOEXCEPT\n" +
Expand Down Expand Up @@ -2826,7 +2802,7 @@ private CharSequence generateMessageFlyweightCode(
" m_actingBlockLength = sbeBlockLength();\n" +
" m_actingVersion = sbeSchemaVersion();\n" +
" m_position = sbeCheckPosition(m_offset + m_actingBlockLength);\n" +
"%19$s" +
"%18$s" +
" return *this;\n" +
" }\n\n" +

Expand All @@ -2847,12 +2823,10 @@ private CharSequence generateMessageFlyweightCode(
" m_actingBlockLength = sbeBlockLength();\n" +
" m_actingVersion = sbeSchemaVersion();\n" +
" m_position = sbeCheckPosition(m_offset + m_actingBlockLength);\n" +
"%19$s" +
"%18$s" +
" return *this;\n" +
" }\n\n" +

"%20$s" +

" %10$s &wrapForDecode(\n" +
" char *buffer,\n" +
" const std::uint64_t offset,\n" +
Expand All @@ -2866,7 +2840,7 @@ private CharSequence generateMessageFlyweightCode(
" m_actingBlockLength = actingBlockLength;\n" +
" m_actingVersion = actingVersion;\n" +
" m_position = sbeCheckPosition(m_offset + m_actingBlockLength);\n" +
"%21$s" +
"%20$s" +
" return *this;\n" +
" }\n\n" +

Expand Down Expand Up @@ -2903,7 +2877,7 @@ private CharSequence generateMessageFlyweightCode(

" SBE_NODISCARD std::uint64_t decodeLength() const\n" +
" {\n" +
" %10$s skipper(m_buffer, m_offset, m_bufferLength, sbeBlockLength(), m_actingVersion%14$s);\n" +
" %10$s skipper(m_buffer, m_offset, m_bufferLength, sbeBlockLength(), m_actingVersion);\n" +
" skipper.skip();\n" +
" return skipper.encodedLength();\n" +
" }\n\n" +
Expand Down Expand Up @@ -2940,14 +2914,13 @@ private CharSequence generateMessageFlyweightCode(
generateConstructorsAndOperators(className, fieldPrecedenceModel),
formatClassName(headerType),
semanticVersion,
codecStateArgument,
generateFieldOrderStateEnum(fieldPrecedenceModel),
generateLookupTableDeclarations(fieldPrecedenceModel),
generateFieldOrderStateMember(fieldPrecedenceModel),
generateAccessOrderErrorType(fieldPrecedenceModel),
generateEncoderWrapListener(fieldPrecedenceModel),
generateDecoderWrapListener(fieldPrecedenceModel),
generateDecoderWrapListenerCall(fieldPrecedenceModel),
generateCodecStateTransitionForWrappingLatestVersion(fieldPrecedenceModel),
generateOnWrappedListener(fieldPrecedenceModel),
generateCodecStateTransitionForWrapping(fieldPrecedenceModel),
generateHiddenCopyConstructor(" ", className));
}

Expand Down Expand Up @@ -3118,7 +3091,7 @@ private static CharSequence generateFieldOrderStateMember(final FieldPrecedenceM
return sb;
}

private static CharSequence generateDecoderWrapListener(final FieldPrecedenceModel fieldPrecedenceModel)
private static CharSequence generateOnWrappedListener(final FieldPrecedenceModel fieldPrecedenceModel)
{
if (null == fieldPrecedenceModel)
{
Expand All @@ -3131,7 +3104,7 @@ private static CharSequence generateDecoderWrapListener(final FieldPrecedenceMod
}

final StringBuilder sb = new StringBuilder();
sb.append(INDENT).append("void onWrapForDecode(std::uint64_t actingVersion)\n")
sb.append(INDENT).append("void onWrapped(std::uint64_t actingVersion)\n")
.append(INDENT).append("{\n");

final MutableBoolean actingVersionCanBeTooLowToBeValid = new MutableBoolean(true);
Expand Down Expand Up @@ -3168,7 +3141,7 @@ private static CharSequence generateDecoderWrapListener(final FieldPrecedenceMod
}


private CharSequence generateDecoderWrapListenerCall(final FieldPrecedenceModel fieldPrecedenceModel)
private CharSequence generateCodecStateTransitionForWrapping(final FieldPrecedenceModel fieldPrecedenceModel)
{
if (null == fieldPrecedenceModel)
{
Expand All @@ -3185,10 +3158,12 @@ private CharSequence generateDecoderWrapListenerCall(final FieldPrecedenceModel
return sb;
}

return generateAccessOrderListenerCall(fieldPrecedenceModel, TWO_INDENT, "onWrapForDecode", "actingVersion");
return generateAccessOrderListenerCall(fieldPrecedenceModel, TWO_INDENT, "onWrapped", "actingVersion");
}

private CharSequence generateEncoderWrapListener(final FieldPrecedenceModel fieldPrecedenceModel)
private CharSequence generateCodecStateTransitionForWrappingLatestVersion(
final FieldPrecedenceModel fieldPrecedenceModel
)
{
if (null == fieldPrecedenceModel)
{
Expand Down
93 changes: 93 additions & 0 deletions sbe-tool/src/test/cpp/FieldAccessOrderCheckTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* limitations under the License.
*/

#include <sstream>
#include "gtest/gtest.h"
#include "gmock/gmock.h"
#include "order_check/MultipleVarLength.h"
Expand Down Expand Up @@ -4955,3 +4956,95 @@ TEST_F(FieldAccessOrderCheckTest, allowsSkippingFutureGroupWhenDecodingFromVersi
EXPECT_EQ(decoder.a(), 42);
EXPECT_EQ(decoder.getBAsString(), "abc");
}

TEST_F(FieldAccessOrderCheckTest, worksWithWrappingConstructors1)
{
MultipleVarLength encoder(m_buffer, BUFFER_LEN);
encoder.a(42);
encoder.putB("abc");
encoder.putC("def");
encoder.checkEncodingIsComplete();

MultipleVarLength decoder(m_buffer, BUFFER_LEN);
EXPECT_EQ(decoder.a(), 42);
EXPECT_EQ(decoder.getBAsString(), "abc");
EXPECT_EQ(decoder.getCAsString(), "def");
}

TEST_F(FieldAccessOrderCheckTest, worksWithWrappingConstructors2)
{
MultipleVarLength encoder(
m_buffer,
BUFFER_LEN,
MultipleVarLength::sbeBlockLength(),
MultipleVarLength::sbeSchemaVersion()
);
encoder.a(42);
encoder.putB("abc");
encoder.putC("def");
encoder.checkEncodingIsComplete();

MultipleVarLength decoder(
m_buffer,
BUFFER_LEN,
MultipleVarLength::sbeBlockLength(),
MultipleVarLength::sbeSchemaVersion()
);
EXPECT_EQ(decoder.a(), 42);
EXPECT_EQ(decoder.getBAsString(), "abc");
EXPECT_EQ(decoder.getCAsString(), "def");
}

TEST_F(FieldAccessOrderCheckTest, worksWithWrappingConstructors3)
{
MultipleVarLength encoder(
m_buffer,
OFFSET,
BUFFER_LEN,
MultipleVarLength::sbeBlockLength(),
MultipleVarLength::sbeSchemaVersion()
);
encoder.a(42);
encoder.putB("abc");
encoder.putC("def");
encoder.checkEncodingIsComplete();

MultipleVarLength decoder(
m_buffer,
OFFSET,
BUFFER_LEN,
MultipleVarLength::sbeBlockLength(),
MultipleVarLength::sbeSchemaVersion()
);
EXPECT_EQ(decoder.a(), 42);
EXPECT_EQ(decoder.getBAsString(), "abc");
EXPECT_EQ(decoder.getCAsString(), "def");
}

TEST_F(FieldAccessOrderCheckTest, worksWithInsertionOperator)
{
MultipleVarLength encoder(
m_buffer,
OFFSET,
BUFFER_LEN,
MultipleVarLength::sbeBlockLength(),
MultipleVarLength::sbeSchemaVersion()
);
encoder.a(42);
encoder.putB("abc");
encoder.putC("def");
encoder.checkEncodingIsComplete();

MultipleVarLength decoder(
m_buffer,
BUFFER_LEN
);
std::stringstream stream;
stream << decoder;

const std::string expected = "\"a\": 42, \"b\": \"abc\", \"c\": \"def\"";
EXPECT_THAT(stream.str(), HasSubstr(expected));
EXPECT_EQ(decoder.a(), 42);
EXPECT_EQ(decoder.getBAsString(), "abc");
EXPECT_EQ(decoder.getCAsString(), "def");
}

0 comments on commit 859d542

Please sign in to comment.