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

[c++] Improved field generation #631

Merged
merged 1 commit into from
Jan 30, 2019
Merged

[c++] Improved field generation #631

merged 1 commit into from
Jan 30, 2019

Conversation

ksergey
Copy link
Contributor

@ksergey ksergey commented Jan 30, 2019

Added std::string_view getter/setter for string-like fields
Added non-const raw access for array like fields

Added non-const raw access for array like fields
@mjpt777 mjpt777 merged commit 1808341 into real-logic:master Jan 30, 2019
@mjpt777
Copy link
Contributor

mjpt777 commented Jan 30, 2019

I have merged but this has some issues. What if the string or string view is shorter than the array? The Java codec deals with this by zero padding the remainder and doing a min on the copy for length of view and length of array.

@ksergey
Copy link
Contributor Author

ksergey commented Jan 30, 2019

What if the string or string view is shorter than the array?

It's UB. Actually I added std::string_view in the same way as std::string was added. So, user should solve this case by themself.

@ksergey
Copy link
Contributor Author

ksergey commented Jan 30, 2019

I think we could add deactivatable via macros check value.length() == fieldLength() inside putField(...) methods.

@mjpt777
Copy link
Contributor

mjpt777 commented Jan 30, 2019

Identifying this as UB feels wrong and error prone. The existing string approach was also wrong in this.

@tmontgomery
Copy link
Contributor

std::string_view is C++17. I don't think we want to go there just yet.

@mjpt777
Copy link
Contributor

mjpt777 commented Jan 30, 2019

@tmontgomery the code is guarded for C++ 17 so only compiled when available.

@tmontgomery
Copy link
Contributor

And, yes, if the string is short, it should null pad.

@tmontgomery
Copy link
Contributor

@mjpt777 yes, I understand that. I would, though, say that to avoid noise and the fact that there is very little reason to use it yet that there is no compelling reason to do it at this time.

@ksergey
Copy link
Contributor Author

ksergey commented Jan 30, 2019

@tmontgomery why short string should be null padded? Is string getter should be aware of this (i.e. return string with length less than field length)?

@tmontgomery
Copy link
Contributor

It probably should be encoding specific what to pad with. And it should be compatible with what Java does right now.

The getter is always going to be a specific length. It just may have encoding specific nulls, etc. The setter is the one to determine what to do with the string/string_view is too short.

@ksergey
Copy link
Contributor Author

ksergey commented Jan 30, 2019

Nice. We can fix cpp generator for string/string_view. What about const char* setter? Calculate string length (strnlen)? Add the second parametr length? template<size_t N> putField(char const (&value)[N]) ?

@tmontgomery
Copy link
Contributor

I've added length checks and handling via 825e0c4

@tmontgomery
Copy link
Contributor

checks for const char * versions are not needed. Those are the sharp knives. Know how to use them. std:string and std::string_view are for those who need their hands held while they cross the street.

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