On Wed, 24 Apr 2024 01:02:36 GMT, Julian Waters <jwat...@openjdk.org> wrote:
> Currently, on Windows LANG is not assigned to C++ for some code that does use > C++. This just works because link.exe does not bother about what kind of code > it is currently linking. gcc however, does. It doesn't hurt to assign LANG to > C++ as a formality in such cases, which this changeset does. This also > renames LINK_TYPE to LANG, which the original change to remove the TOOLCHAIN > parameter used to do Please revert back to the LINK_TYPE name. As long as it is not used for anything else, this is a better description. If we start to use it to have a broader meaning, we can rename it then. make/hotspot/gensrc/GensrcAdlc.gmk line 45: > 43: endif > 44: else ifeq ($(call isBuildOs, windows), true) > 45: ifeq ($(TOOLCHAIN_TYPE), microsoft) You're kind of sneaking in some of your "support other toolchain than ms on windows" changes here. While it does not matter that much, right now we have the assumption that platform=windows <=> toolchain=microsoft in the entire code base. With that assumption, this change looks strange. So I'd rather not take this in now, but instead do a complete integration of the changes needed to support multiple toolchains on Windows. make/hotspot/lib/CompileGtest.gmk line 98: > 96: -I$(GTEST_FRAMEWORK_SRC)/googlemock/include \ > 97: $(addprefix -I,$(GTEST_TEST_SRC)), \ > 98: CFLAGS_windows := -EHsc, \ Just to clarify: these kind of changes are okay, since for mainline it is equivalent if you do `CFLAGS_windows` or `CFLAGS_microsoft`, so if it helps you I am completely okay with converting one kind of check to another. (It was the double-checking that I objected to.) make/modules/java.desktop/lib/AwtLibraries.gmk line 102: > 100: $(eval $(call SetupJdkLibrary, BUILD_LIBAWT, \ > 101: NAME := awt, \ > 102: LANG := $(if $(filter $(OPENJDK_TARGET_OS), windows), C++, C), \ No, this is not okay. You need to do this as for the LIBJSOUND_LINK_TYPE above. The same goes for the one below, too. ------------- PR Comment: https://git.openjdk.org/jdk/pull/18927#issuecomment-2085199170 PR Review Comment: https://git.openjdk.org/jdk/pull/18927#discussion_r1584720911 PR Review Comment: https://git.openjdk.org/jdk/pull/18927#discussion_r1584722581 PR Review Comment: https://git.openjdk.org/jdk/pull/18927#discussion_r1584723694
