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

Port the Analysis lookup change #177

Closed
wants to merge 8 commits into from
Closed

Port the Analysis lookup change #177

wants to merge 8 commits into from

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Sep 20, 2016

(This PR is implemented on top of #176 (Revert #165 but keep some of the changes). The relevant commit(s) are 8dd3aca onward.)

All it does is add the following methods on PerClasspathEntryLookup:

    /** Provides the Analysis for the given classFile. */
    Maybe<CompileAnalysis> lookupAnalysis(File classFile);

    /** Provides the Analysis for the given binary class name in an external JAR. */
    Maybe<CompileAnalysis> lookupAnalysis(File binaryDependency, String binaryClassName);

    /** Provides the Analysis for the given binary class name. */
    Maybe<CompileAnalysis> lookupAnalysis(String binaryClassName);

This enables a more efficient implementation of Analysis lookup that doesn't involve classpath lookups.

Ref gkossakowski/zinc@8092cd8
Ref gkossakowski/zinc@bd0dde3
Ref gkossakowski/sbt@c45f580
Ref sbt/sbt#2525

/review @gkossakowski, @dwijnand

@romanowski
Copy link
Contributor

Genernally we can't skip classpath checks and focus purly on analysis since then we are failing to capture changes in classpath (e.g. shadowing of classes). Please test your implementation with tests from #175 since failures there was reason why I abandon my previous implementation (from #165 ).

This enables a more efficient implementation of Analysis lookup that
doesn't involve classpath lookups.

Ref gkossakowski/zinc@8092cd8
Ref gkossakowski/zinc@bd0dde3
Ref sbt/sbt#2525
@eed3si9n
Copy link
Member Author

Closing this in favor of #180

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.

None yet

2 participants