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

use reflection to read perprojectinfo.rootPathToResolvedEntries #3124

Merged
merged 6 commits into from
Aug 8, 2024

Conversation

cdietrich
Copy link
Member

@cdietrich
Copy link
Member Author

cc @iloveeclipse

@merks
Copy link
Contributor

merks commented Aug 6, 2024

It is really not sufficient just to use? Just wondering...

	protected IClasspathEntry getResolvedClasspathEntry(IJavaProject javaProject, /* @NonNull */ IPackageFragmentRoot root) throws JavaModelException {
		return root.getResolvedClasspathEntry();
	}

@cdietrich cdietrich force-pushed the cd-jdt-resolved-classpath-entry branch from d90c16c to 7e7ff86 Compare August 7, 2024 05:56
@iloveeclipse
Copy link
Contributor

Any reason to hold this PR open?

@cdietrich
Copy link
Member Author

i need a review

Copy link
Contributor

@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 :-)

}

return result;
}


protected final boolean isJdtCoreVersionAtLeast3390 = JavaCore.getPlugin().getBundle().getVersion().compareTo(new Version(3, 39, 0)) >= 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why this isn't a private static final constant? I know it's nit picky, but I'm just wondering?

Copy link
Member Author

Choose a reason for hiding this comment

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

in case somebody wants to override the method below

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a reason for making it protected. Why not make it static?

Copy link
Contributor

Choose a reason for hiding this comment

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

The validator is very long-living (one instance per Xbase language that is installed). I don't mind making this static or not, but it won't make much of a difference.

protected Map<IPath, IClasspathEntry> getRootPathToResolvedEntries(PerProjectInfo info) {
if (isJdtCoreVersionAtLeast3390) {
try {
Method m = PerProjectInfo.class.getDeclaredMethod("getRootPathToResolvedEntries");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:
We could you MethodHandles.findGetter and MethodHandles.findVirtual to obtain a MethodHandle once and use that throughout the lifecyle of this class.

Copy link
Member Author

Choose a reason for hiding this comment

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

am not familiar with that api

Copy link
Member Author

Choose a reason for hiding this comment

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

#3128
something like this?

Copy link
Contributor

@LorenzoBettini LorenzoBettini left a comment

Choose a reason for hiding this comment

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

I'm fine with the PR, but I seem to understand that this one is superseded by #3128 is that right?

@LorenzoBettini LorenzoBettini mentioned this pull request Aug 8, 2024
2 tasks
@cdietrich
Copy link
Member Author

no the other one is pred against this. so i will chain merge once the builds are through

@cdietrich cdietrich merged commit 7f5d3d5 into main Aug 8, 2024
9 checks passed
@cdietrich cdietrich deleted the cd-jdt-resolved-classpath-entry branch August 8, 2024 13:05
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.

5 participants