On Mon, 14 Nov 2022 01:17:38 GMT, Julian Waters <jwat...@openjdk.org> wrote:

>> src/hotspot/os/windows/os_windows.hpp line 35:
>> 
>>> 33: class Thread;
>>> 34: 
>>> 35: static unsigned __stdcall thread_native_entry(Thread*);
>> 
>> Why was this removed? This is needed to correctly specify the call sequence 
>> for the thread entry routine when used with `_beginThreadex`:
>> https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex?view=msvc-170
>
> I'm not sure I follow, I didn't remove anything here?

Sorry my eyes must be playing tricks on me. ??

Why did you need to add this here?

>> src/hotspot/share/utilities/compilerWarnings.hpp line 47:
>> 
>>> 45: #endif
>>> 46: 
>>> 47: #ifndef PRAGMA_DISABLE_VISCPP_WARNING
>> 
>> Why rename this from `MSVC` to `VISCPP`? IIRC the full name is Microsft 
>> Visual Studio C++, so you new name is not obviously better and the change 
>> just adds noise to the PR. Further `MSVC` matches what MS themselves use and 
>> even the attribute namespace in C++11 is `MSVC`.
>> Update: I see the inconsistency with `compilerWarnings_visCPP.hpp`
>
> Yep, it was renamed since the file is also named VISCPP, and I felt that 
> matching the names was a good style change

I think it is the file that has the "bad" name in this case. :( But okay.

>> src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 37:
>> 
>>> 35: #endif
>>> 36: 
>>> 37: #if defined(__clang_major__) && \
>> 
>> Not clear why this was moved ??
>
> I'm not sure which one you're referring to, but the PRAGMA_DIAG_PUSH/POP was 
> moved up to the top of the header to match compilerWarnings_visCPP.hpp, and 
> PRAGMA_DISABLE_GCC_WARNING_AUX was moved to macros.hpp as the more general 
> PRAGMA macro, since it's useful for all compilers and not just gcc

Okay

-------------

PR: https://git.openjdk.org/jdk/pull/11081

Reply via email to