Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

IR encoding omits deprecated #1050

Closed
klittlepage opened this issue Feb 3, 2025 · 3 comments · Fixed by #1049
Closed

IR encoding omits deprecated #1050

klittlepage opened this issue Feb 3, 2025 · 3 comments · Fixed by #1049

Comments

@klittlepage
Copy link
Contributor

The SBE IR schema sbe-ir.xml defines a field deprecated

<field name="deprecated" id="10" type="deprecatedVersionType"/>

Currently, the Encoding and IrEncoder classes don't correctly handle this field, resulting in uninitialized/invalid values in the serialized IR. This patch sets deprecated and produces serialized IR with deprecated set.

@ZachBray
Copy link
Contributor

ZachBray commented Feb 4, 2025

I agree that deprecated doesn't round trip properly through IR.

I'm not sure if it belongs on Encoding though. It lives on a Field and is already present on Token in the "IR model".

Why not just change IrEncoder to take the existing value from token.deprecated()? Please can you explain your rationale?

diff --git a/sbe-tool/src/main/java/uk/co/real_logic/sbe/ir/IrEncoder.java b/sbe-tool/src/main/java/uk/co/real_logic/sbe/ir/IrEncoder.java
index 82ad99d7..13e311eb 100644
--- a/sbe-tool/src/main/java/uk/co/real_logic/sbe/ir/IrEncoder.java
+++ b/sbe-tool/src/main/java/uk/co/real_logic/sbe/ir/IrEncoder.java
@@ -192,7 +192,8 @@ public class IrEncoder implements AutoCloseable
             .signal(mapSignal(token.signal()))
             .primitiveType(mapPrimitiveType(type))
             .byteOrder(mapByteOrder(encoding.byteOrder()))
-            .presence(mapPresence(encoding.presence()));
+            .presence(mapPresence(encoding.presence()))
+            .deprecated(token.deprecated());

@klittlepage
Copy link
Contributor Author

klittlepage commented Feb 4, 2025

I did it for consistency, thinking that Encoding and IrEncoder contained the same fields, but I see that they contain non-overlapping subsets of token fields. So yes, I agree that it only belongs in IrEncoder. I've updated the PR.

@ZachBray
Copy link
Contributor

ZachBray commented Feb 4, 2025

Merged. Thank you, @klittlepage.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants