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

Regression: objToJSON "nonvoid function does not return a value" error is back #31463

Closed
toddrme2178 opened this issue Jan 30, 2020 · 8 comments · Fixed by #31482
Closed

Regression: objToJSON "nonvoid function does not return a value" error is back #31463

toddrme2178 opened this issue Jan 30, 2020 · 8 comments · Fixed by #31482
Labels
IO JSON read_json, to_json, json_normalize
Milestone

Comments

@toddrme2178
Copy link

There was an issue in 2013, #5326, where initObjToJSON in pandas/_libs/src/ujson/python/objToJSON.c was causing a build error due to it lacking a return value. Specifically, the issue was:

pandas/_libs/src/ujson/python/objToJSON.c: In function ‘initObjToJSON’:
pandas/_libs/src/ujson/python/objToJSON.c:181:1: error: control reaches end of non-void function [-Werror=return-type]

This was fixed at the time in #5334. However, a recent commit, #30710, removed the return value again. This has caused the warning/error to reappear, causing our builds of pandas 1.0.0 to fail. The warning is appearing in your CI builds as well. The difference is that we have it configured as an error.

Would it be possible to set a valid return value for the function? Thank you.

@jorisvandenbossche jorisvandenbossche added this to the 1.0.1 milestone Jan 31, 2020
@jorisvandenbossche
Copy link
Member

cc @alimcmaster1 @WillAyd

@jorisvandenbossche jorisvandenbossche added the IO JSON read_json, to_json, json_normalize label Jan 31, 2020
@alimcmaster1
Copy link
Member

alimcmaster1 commented Jan 31, 2020

Taking a look, the reason for the change was the return value was no longer compatible with numpy master. (Our py37_np_dev build).

What's your use case/why do you need this function to return a value? (Given its really an internal to pandas)

Agree there are some warnings related to this:
https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=27217&view=logs&j=3a03f79d-0b41-5610-1aa4-b4a014d0bc70&t=fe74a338-551b-5fbb-553d-25f48b1836e8&l=686

Think that's due to the fact the signature is to return a void pointer

@WillAyd
Copy link
Member

WillAyd commented Jan 31, 2020

Just out of curiosity why are you changing the setup file to error for this warning specifically? Sure can fix but note that our build isn’t warning free (if you search issue tracker will see another issue for that) so a little risky to modify the setup in until that is achieved

@toddrme2178
Copy link
Author

@WillAyd This isn't a specific thing for this package. We are packing it for openSUSE Linux and changing this warning (and some others) to errors is enforced by the build system for the entire entire distribution.

@WillAyd
Copy link
Member

WillAyd commented Jan 31, 2020

The open issue mentioned is #30609 - in the meantime if there's a way to error on the codes you need while keeping the rest as warnings I think would accept a patch for that.

@toddrme2178
Copy link
Author

@alimcmaster1 It isn't really about a use case, but rather about not allowing undefined, potentially unsafe behavior at the compiler level. See, for example, CERT C coding guidelines MSC37-C:

Using the return value from a non-void function where control reaches the end of the function
without evaluating a return statement can lead to buffer overflow vulnerabilities as well as other
unexpected program behaviors.

@toddrme2178
Copy link
Author

@WillAyd It looks like there is already a patch, #31482

@WillAyd
Copy link
Member

WillAyd commented Jan 31, 2020

That patch fixes the warning but doesn't include the flags you are compiling with. Probably worth proposing that in a separate PR to prevent regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO JSON read_json, to_json, json_normalize
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants