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

Add AST getAll[Supported]Versions and isSupportedVersion #3567

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

akurtakov
Copy link
Contributor

New methods to ease finding all/supported JLS versions. Supported JLS versions helpers are needed in order ease limiting AST support to what compiler supports.

@akurtakov
Copy link
Contributor Author

eclipse-jdt/eclipse.jdt.ui#1948 is an example of usage of new API.

@akurtakov
Copy link
Contributor Author

@iloveeclipse IIRC you added the similar API in JavaCore so would you please take a look.

@@ -1193,7 +1204,7 @@ private AST(int level, boolean previewEnabled) {
false/*isPreviewEnabled*/);
break;
default:
if (level < JLS2_INTERNAL && level > JLS_Latest) {
if (!ALL_VERSIONS.contains(level)) {
Copy link
Member

Choose a reason for hiding this comment

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

Should it be SUPPORTED_VERSIONS ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using SUPPORTED_VERSIONS should happen in #3533 together with switching the mapping to prevent inconsistencies.

Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

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

LGTM, but I've never worked within AST area, so no idea what is supposed to be supported or not. So would be nice if someone familiar with AST could review too,

@akurtakov
Copy link
Contributor Author

I'll wait Monday and push it if no one jumps in.

New methods to ease finding all/supported JLS versions. Supported JLS
versions helpers are needed in order ease limiting AST support to what
compiler supports.
@akurtakov akurtakov merged commit cc53f07 into eclipse-jdt:master Jan 20, 2025
10 checks passed
@stephan-herrmann
Copy link
Contributor

I found this only now.

@akurtakov this adds to the set of locations that need to be touched whenever a new Java version is supported. We already have org.eclipse.jdt.core.dom.AST.JLS_INTERNAL_Latest and getJLSLatest() which should encapsulate the moving target. Why do we now need another explicit list?

stephan-herrmann added a commit to jarthana/eclipse.jdt.core that referenced this pull request Jan 30, 2025
@akurtakov
Copy link
Contributor Author

@stephan-herrmann Here yes but e.g. in eclipse-jdt/eclipse.jdt.ui#1966 it removes one such place (the goal is to have jdt.ui/debug need less or even no changes for adding constants).
Supported versions is also used in #3533 and simplifies AST constructor and removes(automates) one more place in ASTTest(which was stuck to Java 20 !). Overall, for the JDT project it would be a decrease.

@stephan-herrmann
Copy link
Contributor

@stephan-herrmann Here yes but e.g. in eclipse-jdt/eclipse.jdt.ui#1966 it removes one such place (the goal is to have jdt.ui/debug need less or even no changes for adding constants). Supported versions is also used in #3533 and simplifies AST constructor and removes(automates) one more place in ASTTest(which was stuck to Java 20 !). Overall, for the JDT project it would be a decrease.

I didn't argue against providing this API, I only wonder why it needs to be implemented using an explicit list.

@akurtakov
Copy link
Contributor Author

My reasons were:

  • match JavaCore SUPPORTED_VERSION implementation so codebase has a more unified way of handling similar problems
  • keep possibility to e.g. keep only LTS versions support (unlikely but still) e.g at some point 8, 11, 17, 18....n are supported.

@stephan-herrmann
Copy link
Contributor

My reasons were: ...

Thanks.

I had hoped we could do with less duplication, but for now I added the new locations needing updates to https://github.com/eclipse-jdt/eclipse.jdt.core/wiki/Adding-support-for-new-Java-version-and-Features-in-JDT (also JavaCore.allVersions was missing) . It's a pity that this list seems to be growing monotonously, never shrinking.

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.

3 participants