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

sign-compare warning in Marshal.hpp #13

Closed
richey-v opened this issue Feb 5, 2021 · 5 comments
Closed

sign-compare warning in Marshal.hpp #13

richey-v opened this issue Feb 5, 2021 · 5 comments
Labels
enhancement New feature or request

Comments

@richey-v
Copy link
Contributor

richey-v commented Feb 5, 2021

Upgraded from Ubuntu18.04 to Ubuntu20.04 and am now getting a (new) sign-compare warning in Marshall.hpp:

djinni/jni/Marshal.hpp: In static member function ‘static djinni::LocalRef<_jbyteArray*> djinni::Binary::fromCpp(JNIEnv*, const CppType&)’:
djinni/jni/Marshal.hpp:246:29: warning: comparison of integer expressions of different signedness: ‘std::vector<unsigned char>::size_type’ {aka ‘long unsigned int’} and ‘int’ [-Wsign-compare]
  246 |             assert(c.size() <= std::numeric_limits<jsize>::max());
      |                    ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Most likely due to and upgrade of clang. I'm going to try to fix this in a fork and issue a PR. I'll also try to update w/ compiler details.

@richey-v
Copy link
Contributor Author

richey-v commented Feb 5, 2021

clang-10

@richey-v
Copy link
Contributor Author

richey-v commented Feb 5, 2021

personally, I'd like some real error handling, rather than assert... the code below that and the other identical asserts will overflow buffers!

@a4z
Copy link
Contributor

a4z commented Feb 6, 2021

Thanks for the report Will, I agree that warning should not be.
Will be removed. That's the easy part.

About an alternative error strategy, .... that's way more tricky to get right.
In real the checks are not there (release builds), so they are more type of documentation.
But it's probably a good idea to go over them and verify

@a4z a4z added the enhancement New feature or request label Feb 6, 2021
@a4z
Copy link
Contributor

a4z commented Mar 9, 2021

The warning should be removed after the merge of #15 , but there is still the suggestion to remove the assert statement and replace it with something different.

Would you be ok with creating a new issue @richey-v please, for the assert question , and closing this one since the warning is resolved?
This would give a nicer change log history also.

@richey-v
Copy link
Contributor Author

richey-v commented Mar 9, 2021

Warnings resolved by #15. #16 opened for the enhancement effort.

@a4z - could you please add the enhancement and any other labels to #16? I don't see where I can do that.

@richey-v richey-v closed this as completed Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants