RFR(S): JDK-8073584 Native compilation warning in unpack code

[Dmitry Samersoff]

John,

Generally speaking, we should select warnings carefully - not just turn
all of it on.

For instance, I don't see any value of "format is not a string literal"
warning for JDK code.

Please, see also below.


On 2015-02-22 06:20, John Rose wrote:
> On Feb 21, 2015, at 5:38 PM, Kumar Srinivasan <kumar.x.srinivasan at oracle.com> wrote:
>>
>> Hi Dmitry,
>>
>> adding John on this.
>>
>> unpack.cpp
>> What is wrong with the  unary operator ? Does this emit a compiler warning ?
> 
> (It's the ternary operator right?)  


> The problem is that the format string (oh, the cleverness!!) is non-constant.

Yes. Actually sprintf here could be replaced with just a strcpy.

>> -  sprintf(buf, ((uint)e.tag < CONSTANT_Limit)? TAG_NAME[e.tag]: "%d", e.tag);
>> +  if ((uint)e.tag < CONSTANT_Limit) {
>> +    sprintf(buf, "%s", TAG_NAME[e.tag]);
>> +  }
>> +  else {
>> +    sprintf(buf, "%d", e.tag);
>> +  }
>>
>> If you are eliminating the unary operator then, the formatting should be
>>
>> if (.....)
>>   ......
>> else
>>   ......
>> or
>> if (.....) {
>> ....
>> else {  <--- note, please don't introduce new formatting/conventions.
> 
> I agree on that.  Let's be whitespace chameleons.

I'll change it.

> 
>>   ....
>> }
>>
>> main.cpp:
>>
>> +      (void) (fread(&filecrc, sizeof(filecrc), 1, u.infileptr) + 1);
>>
>> UGH. What about other compilers are they ok with this ? This may very well
>> get suppressed for gcc, but might emit warnings on MSVC or SunStudio.

It works for all JDK compilers.

> 
> Surely it would be better to bind the result of fread to a throwaway variable, if there's a warning about unused return value.
>     void* fread_result_ignored =
>          fread(&filecrc, sizeof(filecrc), 1, u.infileptr);


It causes "assigned but unused variable" warning.

-Dmitry

-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.