RFR: 8216154: C4819 warnings at HotSpot sources on Windows

[Leo Korinth]

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
> 
>