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

Cpp master issue 994 #995

Merged
merged 3 commits into from
Nov 10, 2021
Merged

Conversation

sukidog
Copy link

@sukidog sukidog commented Nov 8, 2021

No description provided.

The stringification of a msgpack object shouldn't write the
raw bytes of a binary value. It will likely make the result
unprintable. Just print that it's a binary blob and include
the size. E.g.,

{"data":BIN(1032256)}

EXT is handled similarly but without the size. We now also
print the size.

Issue 994
@redboltz
Copy link
Contributor

redboltz commented Nov 8, 2021

Thank you for quick post.
I cross in the post #994 (comment)
Please see it.

@sukidog
Copy link
Author

sukidog commented Nov 8, 2021

Thank you for quick post. I cross in the post #994 (comment) Please see it.

Solution adjusted as per your feedback. See new commit

bool visit_ext(const char* /*v*/, uint32_t /*size*/) {
m_os << "EXT";
bool visit_ext(const char* v, uint32_t size) {
int type = ((size > 0) && (v[0] >= 0)) ? v[0] : -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe my comment is misleading. If the value of type is -2 the output should be -2. I mean print as singed.

m_os << "EXT";
bool visit_ext(const char* v, uint32_t size) {
int type = ((size > 0) && (v[0] >= 0)) ? v[0] : -1;
m_os << "\"EXT(type:" << type << ",size:" << size << ")\"";
Copy link
Contributor

Choose a reason for hiding this comment

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

See https://github.com/msgpack/msgpack/blob/master/spec.md#ext-format-family
The parameter size is size of type + data.
I think that users expect size of data. So the printed value should size-1.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the code should be

    bool visit_ext(const char* v, uint32_t size) {
        if (size == 0) {
            m_os << "\"EXT(size:0)\"";
        }
        else {
            m_os << "\"EXT(type:" << static_cast<int>(v[0]) << ",size:" << size - 1 << ")\"";
        }

Copy link
Author

Choose a reason for hiding this comment

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

Got it. Thanks. Will upload a change tomorrow morning.

Copy link
Contributor

Choose a reason for hiding this comment

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

NOTE: size==0 on EXT is error. But I choose printing only size. (avoid invalid v[0] access.

@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2021

Codecov Report

Merging #995 (bae76b7) into cpp_master (1f663d1) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@              Coverage Diff               @@
##           cpp_master     #995      +/-   ##
==============================================
- Coverage       85.60%   85.56%   -0.04%     
==============================================
  Files              79       79              
  Lines            5007     5009       +2     
==============================================
  Hits             4286     4286              
- Misses            721      723       +2     

@sukidog
Copy link
Author

sukidog commented Nov 9, 2021

I uploaded the EXT implementation you asked for.

@redboltz
Copy link
Contributor

Looks good to me.
Thank you for the contribution!

@redboltz redboltz merged commit 59f2da6 into msgpack:cpp_master Nov 10, 2021
@sukidog
Copy link
Author

sukidog commented Nov 10, 2021

Thanks for accepting it so quickly!

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.

4 participants