On Tue, 7 Mar 2023 16:44:26 GMT, Julian Waters <[email protected]> wrote:
>> Eclipse is a popular and very well-known IDE in the world of Java >> development, utilized widely in many contexts, by beginners and experienced >> teams alike. Although a relatively lightweight IDE, it features surprisingly >> powerful indexing and code analysis capabilities, as well as useful tools, >> among which are make integration. While the tools it provides are not always >> as sophisticated as other IDEs (IntelliJ IDEA will likely come to mind as >> one such competitor), the simplicity of using it, as well as the reliability >> of this rugged IDE makes up greatly for the slightly less advanced tooling. >> Eclipse requires very little starting infrastructure in the workspace for >> all these features and indexing support as well, which makes it a good >> candidate for developing on the JDK. >> >> This enhancement adds 4 extra targets to the make system for generating a >> basic Eclipse Workspace that provides almost full indexing support for the >> JDK, with varying levels as desired, from a minimalistic option only >> including the Java Virtual Machine's source code, to generating a workspace >> with both Java and C/C++ natures included, which allows for using Eclipse's >> unique ability to quickly swap between Java and C/C++ mode to work on both >> native and Java sources at the same time. Cross Compiling support is >> available, and in its entirety the change touches very little of the >> existing make system, barring its own Makefile within the ide subdirectory. >> >> Indexing capabilities utilizing the enhancement: >> <img width="960" alt="java" >> src="https://user-images.githubusercontent.com/32636402/197784819-67ec7de4-7e27-4f33-b738-59b75a9e4403.PNG";> >> <img width="787" alt="escape" >> src="https://user-images.githubusercontent.com/32636402/197784843-df8621a8-7b0a-42da-86f4-3afb92de0f71.PNG";> > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Address Review and add extra comments Functionally this looks ok to me, just some comment on cosmetics. doc/ide.md line 94: > 92: indexing the Java sources in the JDK (see below), is to enable dark mode > 93: before doing so. Trust us, it looks much better than Eclipse's default > look > 94: and feel. ;) Does this change how Eclipse functions with our sources in any way, or is it just your opinion that it looks better? If the latter, then I don't think it really belongs in this document. make/ide/eclipse/CreateWorkspace.gmk line 40: > 38: include hotspot/lib/JvmFlags.gmk > 39: > 40: # Warning: This file does not have the best formatting! Is this still true? If so, we should fix that so the comment can be removed. After having read through the file. I find quite a few places where broken up lines have 2 space indentation instead of 4. (See https://openjdk.org/groups/build/doc/code-conventions.html) Fixing this should be pretty quick and simple and will greatly improve readability at least for me and Magnus. I will mark some of the typical cases for you. make/ide/eclipse/CreateWorkspace.gmk line 60: > 58: # Taken from JdkNativeCompilation.gmk > 59: FindJavaHeaderDir = \ > 60: $(if $(strip $1),$(wildcard $(SUPPORT_OUTPUTDIR)/headers/$(strip $1))) Indent 4. make/ide/eclipse/CreateWorkspace.gmk line 64: > 62: JAVA_DIRS := $(strip $(foreach module, $(call FindAllModules), \ > 63: $(patsubst $(TOPDIR)/%,%,$(filter-out $(OUTPUTDIR)%, \ > 64: $(call FindModuleSrcDirs, $(module)))))) Indent 4. make/ide/eclipse/CreateWorkspace.gmk line 134: > 132: # This is an annoying bug that has not been fixed for some time now > 133: $1_CLASSPATH += $$(foreach src,$(JAVA_DIRS), \ > 134: <classpathentry > excluding="module-info.java|module-info.java.extra" kind="src" > path="$$(src)"/>$$(NEWLINE)) Indent 4. make/ide/eclipse/CreateWorkspace.gmk line 141: > 139: REPLACEMENTS := \ > 140: @@CLASSPATH@@ => $$($1_CLASSPATH), \ > 141: )) Indent 4. make/ide/eclipse/CreateWorkspace.gmk line 154: > 152: > 153: $1_BUILD_MANAGERS += \ > 154: <buildCommand> \ Indent 4. make/ide/eclipse/CreateWorkspace.gmk line 168: > 166: > 167: $1_NATURES += \ > 168: <nature>org.eclipse.cdt.core.cnature</nature> \ Indent 4. make/ide/eclipse/CreateWorkspace.gmk line 233: > 231: @@SRC@@ => $$($1_NATIVE_SRCS) ; \ > 232: @@MAKE_TARGETS@@ => $$($1_MATCHING_MAKE_TARGETS), \ > 233: )) Suggestion: $$(eval $$(call SetupTextFileProcessing, $1_CREATE_NATIVE_FILE, \ SOURCE_FILES := $(TOPDIR)/make/ide/eclipse/native.template, \ OUTPUT_FILE := $$($1_NATIVE_FILE), \ REPLACEMENTS := \ @@DIR@@ => $$(call FixPath, $(TOPDIR)) ; \ @@ENV@@ => $$($1_ENV) ; \ @@WORKSPACE@@ => $$($1_WORKSPACE_MAJOR) ; \ @@MINOR@@ => $$($1_WORKSPACE_MINOR) ; \ @@MAKE@@ => $$($1_MAKE) ; \ @@SRC@@ => $$($1_NATIVE_SRCS) ; \ @@MAKE_TARGETS@@ => $$($1_MATCHING_MAKE_TARGETS), \ )) make/ide/eclipse/CreateWorkspace.gmk line 315: > 313: @@CSETTINGS@@ => $$($1_CSETTINGS) ; \ > 314: @@CXXSETTINGS@@ => $$($1_CXXSETTINGS), \ > 315: )) Suggestion: $$(eval $$(call SetupTextFileProcessing, $1_CREATE_SETTINGS_FILE, \ SOURCE_FILES := $(TOPDIR)/make/ide/eclipse/settings.template, \ OUTPUT_FILE := $$($1_SETTINGS_FILE), \ REPLACEMENTS := \ @@WORKSPACE@@ => $$($1_WORKSPACE_MAJOR) ; \ @@CSETTINGS@@ => $$($1_CSETTINGS) ; \ @@CXXSETTINGS@@ => $$($1_CXXSETTINGS), \ )) make/ide/eclipse/CreateWorkspace.gmk line 364: > 362: @@NATURES@@ => $$($1_NATURES) ; \ > 363: @@LINKED_RESOURCES@@ => $$($1_LINKED_RESOURCES), \ > 364: )) Suggestion: $$(eval $$(call SetupTextFileProcessing, $1_CREATE_WORKSPACE_FILE, \ SOURCE_FILES := $(TOPDIR)/make/ide/eclipse/workspace.template, \ OUTPUT_FILE := $$($1_WORKSPACE_FILE), \ REPLACEMENTS := \ @@BUILD_MANAGERS@@ => $$($1_BUILD_MANAGERS) ; \ @@NATURES@@ => $$($1_NATURES) ; \ @@LINKED_RESOURCES@@ => $$($1_LINKED_RESOURCES), \ )) make/ide/eclipse/CreateWorkspace.gmk line 390: > 388: SHARED := $(SHARED), \ > 389: )) > 390: endif The lack of indentation makes this hard to read. This is how I would like the first block to look: ifeq ($(WORKSPACE), java) $(eval $(call SetupEclipseWorkspace, SETUP_WORKSPACE, \ NATURE := JAVA, \ SHARED := $(SHARED), \ )) else ... ------------- PR: https://git.openjdk.org/jdk/pull/10853
![https://user-images.githubusercontent.com/32636402/197784819-67ec7de4-7e27-4f33-b738-59b75a9e4403.PNG"](https://user-images.githubusercontent.com/32636402/197784819-67ec7de4-7e27-4f33-b738-59b75a9e4403.PNG"){kind=link}
![https://user-images.githubusercontent.com/32636402/197784843-df8621a8-7b0a-42da-86f4-3afb92de0f71.PNG"](https://user-images.githubusercontent.com/32636402/197784843-df8621a8-7b0a-42da-86f4-3afb92de0f71.PNG"){kind=link}