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

Added semantic version as a static public member #931

Merged
merged 6 commits into from Mar 12, 2023

Conversation

renanleonellocastro
Copy link
Contributor

The semantic version is not currently accessible for the clients using the generated stubs. This pull request adds the semantic version as a static public member in the classes generated by the sbe-tool. It is now possible to obtain the value of the sbe template semantic version direct in the source code using the stubs.

@mikeb01
Copy link
Contributor

mikeb01 commented Mar 7, 2023

Hi, thank you for the PR. Pleas could you have a look at the failing builds.

@vyazelenko
Copy link
Contributor

 Execution failed for task ':verifyJavaIrCodecsInSync'.
> Java Ir codecs are out of sync! Please execute the `generateJavaIrCodecs` task and commit the changes.

You need to run:

./gradlew generateJavaIrCodecs

and commit the changes.

@renanleonellocastro
Copy link
Contributor Author

renanleonellocastro commented Mar 7, 2023

Did I do it correctly? Added another commit with the generated files.

@renanleonellocastro
Copy link
Contributor Author

I will check the C++ compiling errors as soon as possible

@renanleonellocastro
Copy link
Contributor Author

It is fixed now. May you please run again the ci pipeline?

@@ -1992,7 +1993,8 @@ private CharSequence generateMessageFlyweightCode(final String className, final
" static const %1$s SBE_BLOCK_LENGTH = %2$s;\n" +
" static const %3$s SBE_TEMPLATE_ID = %4$s;\n" +
" static const %5$s SBE_SCHEMA_ID = %6$s;\n" +
" static const %7$s SBE_SCHEMA_VERSION = %8$s;\n\n" +
" static const %7$s SBE_SCHEMA_VERSION = %8$s;\n" +
" static SBE_CONSTEXPR const char* SBE_SEMANTIC_VERSION = \"%13$s\";\n\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have a const expression here? Also why not have the type as const char * const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to use std::string and also I removed the const expression. I added it before to be able to declare and initialize the SBE_SEMANTIC_VERSION in one single line but it was failing the msvc compiler so I changed it. Now it is declaring the constant in one line and initializing it out of the class.

@@ -15,6 +15,7 @@
public static final int TEMPLATE_ID = 1;
public static final int SCHEMA_ID = 1;
public static final int SCHEMA_VERSION = 0;
public static final String SEMANTIC_VERSION = "null";
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not feel right. The string should be empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. It is fixed now.

@mjpt777
Copy link
Contributor

mjpt777 commented Mar 10, 2023

The MSVC build is broken.

@renanleonellocastro
Copy link
Contributor Author

renanleonellocastro commented Mar 11, 2023

Thank you guys for the review. I did some changes again based on the PR comments. May we run the pipeline again? Cheers
Following is the gradlew report and cppbuild as well.
image

image

@@ -2039,6 +2042,11 @@ private CharSequence generateMessageFlyweightCode(final String className, final
" return %8$s;\n" +
" }\n\n" +

" SBE_NODISCARD static const std::string sbeSemanticVersion() SBE_NOEXCEPT\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be consistent with sbeSemanticType and use a const char *

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

None yet

4 participants