Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[Java] Using sinceVersion from field instead of type declaration #548

Merged

Conversation

zyulyaev
Copy link
Contributor

@zyulyaev zyulyaev commented Mar 20, 2018

Generated if statement in decoder's methods must use sinceVersion from fields declaration, not type declarations.

@zyulyaev
Copy link
Contributor Author

I also added sinceVersion static methods for composite type members.

<type name="version" primitiveType="uint16"/>
<type name="headerLength" primitiveType="uint16"/>

<type name="extraField" primitiveType="int32" sinceVersion="5"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec states "Message header encoding cannot change."

@tmontgomery
Copy link
Contributor

In general, this looks quite good. Thanks for adding the test!

@tmontgomery
Copy link
Contributor

Should probably remove the extension to the MessageHeader, though.

@mjpt777
Copy link
Contributor

mjpt777 commented Mar 21, 2018

Extending a message header is tricky. The message header itself does not come with its own length fields thus it will break existing code which expects message headers to be constant length. It is a part of the specification I've never been comfortable with. The fact that the schemas cannot cross reference makes this more tricky and the only practical way to do anything cross schema is to relay on fixed message headers and other types like group and var data header encodings.

@zyulyaev
Copy link
Contributor Author

Okay, I removed message header extension, but I still would like to have sinceVersion methods in composite decoder classes, if it is not a problem.
I also made similar changes to enums and bitsets.
BTW, don't you think that for enum fields it would be better to remove NULL_VAL constant instead of null reference?

@zyulyaev zyulyaev force-pushed the better-since-version-treatment branch 2 times, most recently from 1a03c40 to c85b8fb Compare March 21, 2018 12:07
@zyulyaev zyulyaev force-pushed the better-since-version-treatment branch from c85b8fb to 9807e40 Compare March 21, 2018 12:08
@tmontgomery tmontgomery merged commit 858fc9f into real-logic:master Mar 21, 2018
@mjpt777
Copy link
Contributor

mjpt777 commented Mar 21, 2018

@zyulyaev The NULL_VAL and SBE_UNKNOWN make it easy to deal with an enum in a switch statement. Without them you could get a NullPointerException.

@zyulyaev
Copy link
Contributor Author

@mjpt777 Sorry, I meant “return”, not “remove”. And that’s exactly my point. But now generated “if” block returns null reference, not the NULL_VAL for enums.

@mjpt777
Copy link
Contributor

mjpt777 commented Mar 22, 2018

@zyulyaev Yes it would make more sense to return the NULL_VAL from the property getter if not present. Good spot.

@mjpt777
Copy link
Contributor

mjpt777 commented Mar 24, 2018

@zyulyaev I've made a change to return the NULL_VAL like you suggested.

@zyulyaev
Copy link
Contributor Author

@mjpt777 Great, thank you!

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

Successfully merging this pull request may close these issues.

3 participants