Skip to content

Updated the codecs used with latest changes [CTT-713]#1336

Merged
ihsandemir merged 4 commits into
hazelcast:masterfrom
ihsandemir:update_codecs
Oct 21, 2025
Merged

Updated the codecs used with latest changes [CTT-713]#1336
ihsandemir merged 4 commits into
hazelcast:masterfrom
ihsandemir:update_codecs

Conversation

@ihsandemir
Copy link
Copy Markdown
Collaborator

Updated the codecs used with latest changes. Also, updated the code to use these codecs properly with the new introduced parameters.

Associated protocol PR is hazelcast/hazelcast-client-protocol#557

…o use these codecs properly with the new introduced parameters.

Associated protocol PR is hazelcast/hazelcast-client-protocol#557
@ihsandemir ihsandemir added this to the 5.5.0 milestone Oct 20, 2025
@ihsandemir ihsandemir requested review from JackPGreen and yuce October 20, 2025 12:09
@ihsandemir ihsandemir self-assigned this Oct 20, 2025
@ihsandemir ihsandemir changed the title Updated the codecs used with latest changes Updated the codecs used with latest changes [CTT-713] Oct 20, 2025
@JackPGreen
Copy link
Copy Markdown
Contributor

Updated the codecs used with latest changes. Also, updated the code to use these codecs properly with the new introduced parameters.

Could you try to keep the PRs separate, if possible, please?

I.E. are these changes only the autogenerated output of hazelcast/hazelcast-client-protocol#557? or are there additional changes as well?

@ihsandemir ihsandemir enabled auto-merge (squash) October 20, 2025 13:06
@ihsandemir
Copy link
Copy Markdown
Collaborator Author

Updated the codecs used with latest changes. Also, updated the code to use these codecs properly with the new introduced parameters.

Could you try to keep the PRs separate, if possible, please?

I.E. are these changes only the autogenerated output of hazelcast/hazelcast-client-protocol#557? or are there additional changes as well?

No, they are related and needed. I can not separate them.

@JackPGreen
Copy link
Copy Markdown
Contributor

Updated the codecs used with latest changes. Also, updated the code to use these codecs properly with the new introduced parameters.

Could you try to keep the PRs separate, if possible, please?
I.E. are these changes only the autogenerated output of hazelcast/hazelcast-client-protocol#557? or are there additional changes as well?

No, they are related and needed. I can not separate them.

Ok, I've tried splitting it (locally to understand it) but I think there's a problem - the generated client-protocol from hazelcast/hazelcast-client-protocol#557 doesn't match the generated-sources in this PR.

You can see my working in this branch.

Comment thread hazelcast/include/hazelcast/client/internal/version.h Outdated
@ihsandemir
Copy link
Copy Markdown
Collaborator Author

Updated the codecs used with latest changes. Also, updated the code to use these codecs properly with the new introduced parameters.

Could you try to keep the PRs separate, if possible, please?
I.E. are these changes only the autogenerated output of hazelcast/hazelcast-client-protocol#557? or are there additional changes as well?

No, they are related and needed. I can not separate them.

Ok, I've tried splitting it (locally to understand it) but I think there's a problem - the generated client-protocol from hazelcast/hazelcast-client-protocol#557 doesn't match the generated-sources in this PR.

You can see my working in this branch.

I generate and format the code. Is the problem about formatting(clang format) difference or do you see other differences?

Co-authored-by: Jack Green <JackPGreen@Gmail.com>
Copy link
Copy Markdown
Contributor

@JackPGreen JackPGreen left a comment

Choose a reason for hiding this comment

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

I generate and format the code. Is the problem about formatting(clang format) {...}?

I think this makes it very challenging to review - if it's generated code, as long as I'm happy with the generator configuration (i.e. hazelcast/hazelcast-client-protocol#557) and I can validate the code is generated as it should be, I can simply rubber-stamp it.

In this scenario, I can't validate the code is generated as it should be, so instead I need to review the generator configuration and review the generated code as well.

I tried to update the client-protocol template formatting in https://github.com/JackPGreen/hazelcast-client-protocol/tree/clang-formatting to see if I could produce the output heree "out-of-the-box" but it's not (easily) possible as the formatter is line breaking in different positions due to the line length (i.e. a parameter may be declared on the same line as a function or a newline depending on the name of the method - which isn't easy to implement in a template).

I think it would be better simply to leave the formatting of the generated code as-is.

do you see other differences?

I don't think so.

Comment thread hazelcast/src/hazelcast/client/network.cpp
@ihsandemir
Copy link
Copy Markdown
Collaborator Author

I generate and format the code. Is the problem about formatting(clang format) {...}?

I think this makes it very challenging to review - if it's generated code, as long as I'm happy with the generator configuration (i.e. hazelcast/hazelcast-client-protocol#557) and I can validate the code is generated as it should be, I can simply rubber-stamp it.

In this scenario, I can't validate the code is generated as it should be, so instead I need to review the generator configuration and review the generated code as well.

I tried to update the client-protocol template formatting in https://github.com/JackPGreen/hazelcast-client-protocol/tree/clang-formatting to see if I could produce the output heree "out-of-the-box" but it's not (easily) possible as the formatter is line breaking in different positions due to the line length (i.e. a parameter may be declared on the same line as a function or a newline depending on the name of the method - which isn't easy to implement in a template).

I think it would be better simply to leave the formatting of the generated code as-is.

do you see other differences?

I don't think so.

We can integrate clang-format into the generator. the commands is similar to clang-format -i --verbose hazelcast/generated-sources/src/hazelcast/client/protocol/codec/codecs.* but then we need to find how to pass the .clamg-format file in cpp repo to this command at protocol repo, Well we can just copy for the start.

@JackPGreen
Copy link
Copy Markdown
Contributor

We can integrate clang-format into the generator. the commands is similar to clang-format -i --verbose hazelcast/generated-sources/src/hazelcast/client/protocol/codec/codecs.*

🤦

Good point - it's much simpler to actually use the formatter than to try to approximate it.

@ihsandemir
Copy link
Copy Markdown
Collaborator Author

We can integrate clang-format into the generator. the commands is similar to clang-format -i --verbose hazelcast/generated-sources/src/hazelcast/client/protocol/codec/codecs.*

🤦

Good point - it's much simpler to actually use the formatter than to try to approximate it.

Sent the PR for this hazelcast/hazelcast-client-protocol#558

JackPGreen added a commit to JackPGreen/hazelcast-client-protocol that referenced this pull request Oct 21, 2025
The format produced by the generator does not match the style of the codebase, making validation tricky - see hazelcast/hazelcast-cpp-client#1336

Instead, the generator should format the code it produces (using the same formatter as used the C++ client) _as it's produced_.
@ihsandemir ihsandemir merged commit c8533ab into hazelcast:master Oct 21, 2025
46 checks passed
@ihsandemir ihsandemir deleted the update_codecs branch March 30, 2026 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants