RFR (L): 8062370: Various minor code improvements

[Thomas Stüfe]

Hi David

On Fri, Nov 7, 2014 at 8:13 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> On 6/11/2014 11:21 PM, Thomas Stüfe wrote:
>
>> Hi David,
>>
>> Our intend was to always guarantee that the written string is zero
>> terminated, and across platforms to always return the same value (-1) if
>> truncation happened.
>>
>> The original jio_snprintf() did not have any value over plain snprintf()
>> (apart from maybe solving the name problem with "_snprintf" on Windows).
>> But having a dedicated wrapper function around snprintf() suggests some
>> added value, and I always thought that value was supposed to be zero
>> termination. If that is not intended, it would be better to use the
>> plain snprintf instead, because at least C programmers then know what to
>> expect.
>>
>> I also found very few cases where the return code of jio_snprintf() was
>> actually checked and truncation handled correctly. Which would be
>> difficult too, because the return code differed for truncation between
>> windows and Posix.
>>
>> Bottomline, I think it would be better if jio_snprintf() were to always
>> zero-terminate, guaranteed.
>>
>
> I don't disagree, but we needed to make sure that everyone was on the same
> page before this change occurred. It is not unreasonable that Windows
> developers writing windows code assumed windows semantics. As a Reviewer I
> should have paid more attention to this aspect.
>
>
I understand this.

In the SAP JVM we have regression tests for C/C++ code, similar to jprt,
but on C function level. Nothing fancy, just some big test functions which
test our C APIs for regressions like this. That code is just compiled into
the hotspot and can be executed with a command line switch, but gets
excluded in release builds.
Is there something similar for the OpenJDK? If yes, I would provide test
functions for jio_snprintf. If no, would it be worth contributing?


> Also if we are going to implement this behaviour then it really needs to
> be clearly documented - both in terms of null-termination and return value
> - and commented in the code (anyone knowing *NIX or Windows behaviour alone
> will be quite perplexed by it I think).


You are right, we should add comments.


> Further, returning -1 to indicate truncation while familiar to windows
> programmers might be frustrating to others who would like to know what size
> buffer was needed - lowest common denominator I know :(
>
> As you said, lowest common denominator. Not easy to implement unless you
implement the whole printf routine yourself (we have done this in our
tracing subsystem to get unified behaviour on all platforms). I also did
not find any usage like this.

And as you note most uses of this function have a "dont care" attitude to
> truncation - which makes it hard to spot if there may be other lurking
> truncation issues in the windows code.
>

Unfortunately I think this is not Windows-specific.
Would it be worth to  assert this case (assert if vsnprintf returns exactly
count bytes) ?

Kind Regards, Thomas