RFR: 8216154: C4819 warnings at HotSpot sources on Windows

[Yasumasa Suenaga]

Hi Leo,

> 1) Should not all of those files be fixed?


> ./src/hotspot/cpu/x86/macroAssembler_x86_sha.cpp:                          C source, UTF-8 Unicode text
> ./src/hotspot/share/oops/method.cpp:                                                                   C source, UTF-8 Unicode text

Non-ASCII character(s) in comment line.


> ./src/hotspot/cpu/aarch64/macroAssembler_aarch64_trig.cpp:                          C source, UTF-8 Unicode text

It's not used for Windows (AArch64).


> ./src/hotspot/share/gc/parallel/gcTaskManager.hpp:                          data

I couldn't find why `file` detects it as "data". So I have no idea for it.


> ./src/hotspot/share/code/codeHeapState.cpp:                          C source, UTF-8 Unicode text

It's ASCII file on my laptop :-)


> ./test/hotspot/gtest/utilities/test_json.cpp:                                                                   C source, UTF-8 Unicode text

It's test code.


> 2) Why remove warning (in one file, methodMatcher.cpp) instead of changing encoding?
> 3) methodMatcher.cpp seems to be pure ASCII, why the change in that file at all?

The error occurs about `RANGE0`. It has binary data, so it might not be able to change encoding.


Thanks,

Yasumasa


On 2019/01/07 23:32, Leo Korinth wrote:
> Hi!
> 
> Running: find -name "*.[ch]pp" | xargs file | grep -v ASCII
> ./src/hotspot/cpu/x86/macroAssembler_x86_sha.cpp:                          C source, UTF-8 Unicode text
> ./src/hotspot/cpu/aarch64/macroAssembler_aarch64_trig.cpp:                          C source, UTF-8 Unicode text
> ./src/hotspot/share/gc/parallel/gcTaskManager.hpp:                          data
> ./src/hotspot/share/code/codeHeapState.cpp:                          C source, UTF-8 Unicode text
> ./src/hotspot/share/oops/method.cpp:                                                                   C source, UTF-8 Unicode text
> ./test/hotspot/gtest/utilities/test_json.cpp:                                                                   C source, UTF-8 Unicode text
> 
> 
> The single hpp file seems fine though (just file not understanding that it is a source file).
> 
> Some questions, as it seems like I am missing something.
> 1) Should not all of those files be fixed?
> 2) Why remove warning (in one file, methodMatcher.cpp) instead of changing encoding?
> 3) methodMatcher.cpp seems to be pure ASCII, why the change in that file at all?
> 
> $ grep --color -P -n "[^[:ascii:]]" is a good way to find the problematic line.
> 
> Thanks, Leo
> 
> On 07/01/2019 14:20, Thomas Schatzl wrote:
>> Hi,
>>
>> On Mon, 2019-01-07 at 21:36 +0900, Yasumasa Suenaga wrote:
>>> Hi Kim,
>>>
>>> On 2019/01/07 7:18, Kim Barrett wrote:
>>>>> On Jan 6, 2019, at 12:54 PM, Kim Barrett <kim.barrett at oracle.com>
>>>>> wrote:
>>>>>
>>>>>> On Jan 6, 2019, at 7:53 AM, Yasumasa Suenaga <
>>>>>> yasuenag at gmail.com> wrote:
>>>>>>
>>>>>> Hi Kim,
>>>>>>
>>>>>> Thank you for your comment.
>>>>>> I uploaded new webrev to use pragma warning push/pop:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8216154/webrev.01/
>>>>>>
>>>>>>
>>>>>> Please review again.
>>>>>
>>>>> Looks good.
>>
>> I tried to verify these problems on these two files as suggested with
>> "iconv -f US-ASCII -t UTF8 <file>" which errored out on
>> codeHeapState.cpp as expected but there has been no error with
>> methodMatcher.cpp. Am I doing something wrong?
>>
>> I am fine with that change if it is really needed for successful
>> compliation :) I just can't find the non-US-ASCII character used in the
>> line indicated by the error message.
>>
>>>>
>>>> It later occurred to me to wonder whether _WINDOWS was the right
>>>> macro to conditionalize on.  All other uses of #pragma warning
>>>> push/pop (there are 5 in HotSpot) use _MSC_VER.
>>>
>>> I updated webrev to use _MSC_VER. Is it ok?
>>>
>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8216154/webrev.02/
>>>
>>
>> Please add a "// warning C4189: The file contains a character that
>> cannot be represented in the current code page" comment above or next
>> to the pragma warning(disable) declaration.
>>
>> Not many people know the VC warning numbers by default...
>>
>> Looks good otherwise, I do not need a re-review for this comment
>> change.
>>
>> Thanks,
>>    Thomas
>>
>>