lib: deprecate G_snprintf() and replace with C99 snprintf() #4189
Conversation
Beginning with UCRT in Visual Studio 2015 and Windows 10, the behaviour of snprintf (and vsnprintf) is now C99 standard conformant. Thus, the output is now always null-terminated. The justification for a internal fix for that is no longer needed.
|
When you say deprecate, is it only in some docs, or by using super strict compiler flags we can make our builds in CI fail for it, to not introduce back some usages of it? Users compiling with normal flags would still be able to successfully build. |
|
Do I understand correctly that the following in Microsoft documentation is a typo?
|
That's most likely. Further down you can read "snprintf always stores a terminating NULL character, truncating the output if necessary.". |
I was foremost thinking in documentation terms (it will end up listed here). |
|
Among Addons only r.sim.water.mp make use of G_snprintf(), whereas i.wavelet is the only one using snprintf(). |
No need to change that. It should be removed given the parallel version is now in core. |
That's a bit what I was thinking about. I didn't know if it existed in C/C++. I've tried to read a bit about it, and it doesn't seem like it would be as easy as in C# where we could've placed a conditional compilation around the attribute to only appear when the compiler supports it, at a single place, and all usages of that function would appear as warnings during compilation. So it's fine with me to leave it for later. |
There was a problem hiding this comment.
I'll resume what I understand from all this, to do like rubberducking, but for my comprehension of what I'm approving:
From all the changed files, there are two patterns used plus two exceptions.
- For C files, stdio is imported if not imported yet, then G_as printer is replaced with snprintf.
- For C++ files, cstdio is imported if not imported yet, then G_snprintf is replaced with std::snprintf.
For the two exceptions:
- the file that used to host G_snprintf is changed to use snprintf
- raster3d/r3.out.bin/main.c: this is the only file to double check. It has a unique change (with respect to the other changes in this PR). It declares the character array for the output filename with a size of GNAME_MAX instead of being a char pointer. Further below, it checks if there is an answer to the output parameter. If so, it is used as is with an empty string extension appended. If the output parameter doesn't have an answer, the
nameis used instead, and appended with a.binextension. The complete snprintf call, since it returns the size (characters) that the buffer would have taken if it was successful, is used to check if it is bigger than the size of the outfile buffer (that was defined in this PR to GNAME_MAX). An error message is shown if the name is too long, and addition of this PR.
Ah, the rubberducking worked, there's a situation that isn't handled here.
If we look at the cppreference page, https://en.cppreference.com/w/cpp/io/c/fprintf
- Number of characters that would have been written for a sufficiently large buffer if successful (not including the terminating null character), or a negative value if an error occurred. Thus, the (null-terminated) output has been completely written if and only if the returned value is nonnegative and less than
buf_size.
The new logic seems to miss what happens when snprintf returns a negative value.
If it wasn't for that last point, I was approving this PR.
This is not correct, it still uses
The change of G_snprintf to C99 snprintf, made the compiler aware of a bug. The size of a pointer was checked instead of the size of the array. I addressed this and added a check whether the output string was truncated. This is the common practice (when checked at all!). This is not the PR to check for all potential errors. |
You're right, I missed the v when writing back from memory and at my first reading.
So you are saying that it doesn't need to be checked for negative return value at all then? Or you do not want it here? |
|
Was it almost ready? |
It is ready. |
* lib: deprecate G_snprintf() and replace with C99 snprintf() Beginning with UCRT in Visual Studio 2015 and Windows 10, the behaviour of snprintf (and vsnprintf) is now C99 standard conformant. Thus, the output is now always null-terminated. The justification for a internal fix for that is no longer needed. * r3.out.bin: fix -Werror=sizeof-pointer-memaccess GCC warning * cast explicitly * add missing #include
* lib: deprecate G_snprintf() and replace with C99 snprintf() Beginning with UCRT in Visual Studio 2015 and Windows 10, the behaviour of snprintf (and vsnprintf) is now C99 standard conformant. Thus, the output is now always null-terminated. The justification for a internal fix for that is no longer needed. * r3.out.bin: fix -Werror=sizeof-pointer-memaccess GCC warning * cast explicitly * add missing #include
* lib: deprecate G_snprintf() and replace with C99 snprintf() Beginning with UCRT in Visual Studio 2015 and Windows 10, the behaviour of snprintf (and vsnprintf) is now C99 standard conformant. Thus, the output is now always null-terminated. The justification for a internal fix for that is no longer needed. * r3.out.bin: fix -Werror=sizeof-pointer-memaccess GCC warning * cast explicitly * add missing #include
* lib: deprecate G_snprintf() and replace with C99 snprintf() Beginning with UCRT in Visual Studio 2015 and Windows 10, the behaviour of snprintf (and vsnprintf) is now C99 standard conformant. Thus, the output is now always null-terminated. The justification for a internal fix for that is no longer needed. * r3.out.bin: fix -Werror=sizeof-pointer-memaccess GCC warning * cast explicitly * add missing #include
Beginning with UCRT in Visual Studio 2015 and Windows 10, the behaviour of
snprintf()(andvsnprintf()) is now C99 standard conformant (source).Thus, the output is now always null-terminated. The justification for an internal fix for that is no longer needed.
I should add that the code base is already using snprintf() in a number of places.