Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove obsoleted code in CompilerOptions #2924

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

iloveeclipse
Copy link
Member

Found during the review of #2551

  • CompilerOptions.originalComplianceLevel is obsoleted and can be removed + code in TypeConverter using it should be cleaned up.
  • CompilerOptions.originalSourceLevel is obsoleted and can be removed + code in JDT that uses it should be cleaned up

Fixes #2760

@iloveeclipse
Copy link
Member Author

@srikanth-sankaran : you've pointed me to the code in question on original issue #2551, could you please check if that is what you've envisioned or more code could/should be removed?

@srikanth-sankaran
Copy link
Contributor

@srikanth-sankaran : you've pointed me to the code in question on original issue #2551, could you please check if that is what you've envisioned or more code could/should be removed?

The changes look reasonable - studying the test failures ...

@srikanth-sankaran
Copy link
Contributor

With this additional fix, these 6 failures go away:

diff --git a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/compiler/parser/TypeConverter.java b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/compiler/parser/TypeConverter.java
index a8a918f..d5c302d 100644
--- a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/compiler/parser/TypeConverter.java
+++ b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/compiler/parser/TypeConverter.java
@@ -427,32 +427,22 @@
 					identCount ++;
 					break;
 				case '<' :
-					/* We need to convert and preserve 1.5 specific constructs either if compliance is 1.5 or above,
-					   or the caller has explicitly requested generics to be included. The parameter includeGenericsAnyway
-					   should be used by the caller to signal that in the calling context generics information must be
-					   internalized even when the requesting project is 1.4. But in all cases, we must skip over them to
-					   see if there are any applicable type fragments after the type parameters: i.e we just aren't done
-					   having seen a '<' in 1.4 mode.
 
-					   Because of the way type signatures are encoded, TypeConverter.decodeType(String, int, int, int) is immune
-					   to this problem. See https://bugs.eclipse.org/bugs/show_bug.cgi?id=325633
-					 */
-					if (includeGenericsAnyway) {
-						if (fragments == null) fragments = new ArrayList(2);
-					}
+					if (fragments == null) fragments = new ArrayList(2);
+
 					nameFragmentEnd = this.namePos-1;
-					if (includeGenericsAnyway) {
-						char[][] identifiers = CharOperation.splitOn('.', typeName, nameFragmentStart, this.namePos);
-						fragments.add(identifiers);
-					}
+
+					char[][] identifiers = CharOperation.splitOn('.', typeName, nameFragmentStart, this.namePos);
+					fragments.add(identifiers);
+
 					this.namePos++; // skip '<'
 					TypeReference[] arguments = decodeTypeArguments(typeName, length, start, end, includeGenericsAnyway); // positionned on '>' at end
-					if (includeGenericsAnyway) {
-						fragments.add(arguments);
-						identCount = 0;
-						nameFragmentStart = -1;
-						nameFragmentEnd = -1;
-					}
+
+					fragments.add(arguments);
+					identCount = 0;
+					nameFragmentStart = -1;
+					nameFragmentEnd = -1;
+
 					// next increment will skip '>'
 					break;
 			}

Copy link
Contributor

@srikanth-sankaran srikanth-sankaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the proposed additional fix, the changes look good

@srikanth-sankaran
Copy link
Contributor

Do you want me to take this forward, @iloveeclipse ??

@iloveeclipse
Copy link
Member Author

Do you want me to take this forward, @iloveeclipse ??

Would be great, yes. Feel free to force push to the branch.

Found during the review of eclipse-jdt#2551

- CompilerOptions.originalComplianceLevel is obsoleted and can be
removed + code in TypeConverter using it should be cleaned up.
- CompilerOptions.originalSourceLevel is obsoleted and can be removed +
code in JDT that uses it should be cleaned up

Fixes eclipse-jdt#2760
@srikanth-sankaran srikanth-sankaran added this to the 4.34 M1 milestone Sep 24, 2024
@srikanth-sankaran srikanth-sankaran merged commit 59bc0be into eclipse-jdt:master Sep 24, 2024
9 checks passed
@iloveeclipse iloveeclipse deleted the issue_2760 branch September 24, 2024 11:42
@iloveeclipse
Copy link
Member Author

Thanks Srikanth!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove obsoleted code in CompilerOptions
2 participants