-
Notifications
You must be signed in to change notification settings - Fork 262
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
sprintf -> snprintf #2691
sprintf -> snprintf #2691
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in principle a good change, but it involves too many files in one PR.
I would suggest breaking it up to do one directory at a time, E,g, for libdap4, libhdf5, etc.
Breaking what up? The big commit? By my count that would be 17 commits. Is this to make bisecting possible regressions easier? (Trying to understand why...) |
Agreed this is a lot of files, but given the consistent nature of the change, it is something I can commit to spending some time going through. The main issue will be if there are any strings which should not be |
Also the last commit is a WIP... A public API needs to change to be given the length of a buffer... I used a stupid placeholder name for the new function... What would you like to call it? |
@WardF @edwardhartnett @DennisHeimbigner I'd like to fix up the last of the commits in this PR... How does NetCDF deal with deprecating and replacing APIs? i.e. for this part: -extern void cdRel2Iso(cdCalenType timetype, char* relunits, int separator, double reltime, char* chartime);
+extern void cdRel2IsoNew(cdCalenType timetype, char* relunits, int separator, double reltime, char* chartime, size_t chartime_size);
+extern void cdRel2Iso(cdCalenType timetype, char* relunits, int separator, double reltime, char* chartime); // deprecated
Thanks. |
In the hopes of actually getting this merged, I moved the WIP part to a new PR: #2743 This one is now ready to merge, after review. |
Merged conflicts fixed. @WardF @DennisHeimbigner this one is ready to merge IMHO, pending your reviews. |
Testing on Windows for completeness sake; our current CI builds but doesn't run the tests, due to hangs on the underlying systems. It's been a while, I need to double-check that again, we really need to get Windows CI running again (not just building). |
@WardF did your Windows test pass? @edwardhartnett are you good with these changes now? |
@seanm I will take a look at this; given the size of the merge, it will take a little bit (also combined with the backlog I need to go through, and the deadline to submit my pre-recorded talk to AGU on Friday) but this and others will be reviewed before the upcoming 4.9.3. Thanks! |
In all these cases the size of the buffer can be computed with sizeof.
…ng size One case required slightly complicated accounting of how much space is left in the buffer.
The last commit needs discussion of how to deprecate an API and replace it with another, and to choose its name...