On Thu, 10 Nov 2022 06:20:41 GMT, Julian Waters <jwat...@openjdk.org> wrote:
> After [JDK-8292008](https://bugs.openjdk.org/browse/JDK-8292008) and > [JDK-8247283](https://bugs.openjdk.org/browse/JDK-8247283), some C and C++ > code across the JDK can be replaced and simplified with cleaner language > features that were previously not available due to required compatibility > with the now unsupported Visual C++ 2017 compiler. These cleanups were > highlighted by the very briefly integrated 8296115 > > No changes to the behaviour of the JDK has resulted in any way from this > commit This looks good in general. It is a pity there is so much simple moving of where "attributes" are listed, as it makes it look like the changes are more extensive than they really are - that said I prefer to see the attributes appear before a function/method signature rather than after, or somewhere in-between. A few other comments below. Thanks. make/autoconf/flags-cflags.m4 line 632: > 630: if test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = xclang; > then > 631: STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections > -fdata-sections \ > 632: -DJNIEXPORT='[[gnu::visibility(\"hidden\")]]'" So IIUC we now use attributes via the C++11 syntax rather than compiler-specific syntax - even where the C++11 syntax is referring to a compiler specific attribute. Is that right? src/hotspot/os/linux/os_perf_linux.cpp line 233: > 231: * Ensure that 'fmt' does _NOT_ contain the first two "%d %s" > 232: */ > 233: SCANF_ARGS(2, 0) static int vread_statdata(const char* procfile, > _SCANFMT_ const char* fmt, va_list args) { If `SCANF_ARGS` can/must come first then I suggest adding a newline after it so the method signature is easier to spot. Applied everywhere of course. 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 src/hotspot/share/cds/filemap.hpp line 482: > 480: > 481: // Errors. > 482: ATTRIBUTE_PRINTF(1, 2) static void fail_stop(const char *msg, ...); Again I suggest a newline after `ATTRIBUTE_PRINTF` 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` src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 37: > 35: #endif > 36: > 37: #if defined(__clang_major__) && \ Not clear why this was moved ?? src/hotspot/share/utilities/debug.hpp line 172: > 170: void report_fatal(VMErrorType error_type, const char* file, int line, > const char* detail_fmt, ...) ATTRIBUTE_PRINTF(4, 5); > 171: void report_vm_out_of_memory(const char* file, int line, size_t size, > VMErrorType vm_err_type, > 172: const char* detail_fmt, ...) > ATTRIBUTE_PRINTF(5, 6); Why were the ATTRIBUTE_PRINTFs removed? ------------- PR: https://git.openjdk.org/jdk/pull/11081
