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 mirror holder #100

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Remove mirror holder #100

wants to merge 4 commits into from

Conversation

Ao-senXiong
Copy link
Contributor

Fixes #38

Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

Thanks for looking into how to clean this up!

@@ -117,8 +120,7 @@ public void handleBinaryTree(AnnotatedTypeMirror atm, BinaryTree binaryTree) {
}
}

private static class UniverseInferencePropagationTreeAnnotater
extends PropagationTreeAnnotator {
private class UniverseInferencePropagationTreeAnnotater extends PropagationTreeAnnotator {
public UniverseInferencePropagationTreeAnnotater(AnnotatedTypeFactory atypeFactory) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this a reference to the enclosing class? Or to the realTypeFactory?
By making the class non-static, there is now another type factory in scope and you access the qualifiers through that...


public UniverseInferenceVisitor(
UniverseInferenceChecker checker,
InferenceChecker ichecker,
BaseAnnotatedTypeFactory factory,
boolean infer) {
super(checker, ichecker, factory, infer);

ANY = AnnotationBuilder.fromClass(elements, Any.class);
Copy link
Member

Choose a reason for hiding this comment

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

Yet another copy. That's the reason why the "holder" contained all these fields. Instead of duplicating them, see whether you can make the access nice through one copy in the real type factory.

if (elt.getKind() == ElementKind.CONSTRUCTOR
&& type instanceof AnnotatedTypeMirror.AnnotatedExecutableType) {
((AnnotatedTypeMirror.AnnotatedExecutableType) type)
.getReturnType()
.addMissingAnnotations(Arrays.asList(UniverseAnnotationMirrorHolder.SELF));
.addMissingAnnotations(Arrays.asList(atm.SELF));
Copy link
Member

Choose a reason for hiding this comment

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

Please rename atm to realTypeFactory or something else more suitable (all other uses of atm in this file are for AnnotatedTypeMirror, with some uses confusingly being named type).

|| AnnotatedTypes.containsModifier(atpb.getLowerBound(), LOST)) {
if (AnnotatedTypes.containsModifier(
atpb.getUpperBound(),
((UniverseAnnotatedTypeFactory) atypeFactory).LOST)
Copy link
Member

Choose a reason for hiding this comment

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

You have a few casts like that, maybe add a field.

@@ -32,28 +25,37 @@ protected AnnotationMirror combineAnnotationWithAnnotation(
assert receiverAnnotation != null;
assert declaredAnnotation != null;

if (AnnotationUtils.areSame(receiverAnnotation, SELF)) {
if (AnnotationUtils.areSame(
receiverAnnotation, ((UniverseAnnotatedTypeFactory) atypeFactory).SELF)) {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about extracting the casts by adding a field or even just a local variable here.

public UniverseCombineConstraintEncoder(
Lattice lattice, Map<AnnotationMirror, Integer> typeToInt) {
super(lattice, typeToInt);
Set<AnnotationMirror> allTypes = typeToInt.keySet();
for (AnnotationMirror am : allTypes) {
Copy link
Member

Choose a reason for hiding this comment

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

What is this doing?

@wmdietl wmdietl assigned Ao-senXiong and unassigned wmdietl Jun 28, 2024
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.

Lattice class cleanup
2 participants