Skip to content

Conversation

rudkx
Copy link
Contributor

@rudkx rudkx commented Oct 20, 2018

No description provided.

rudkx added 3 commits October 19, 2018 12:57
Change the order of statements but nothing functional.
Unnest code by splitting it out into lambdas.
@rudkx rudkx requested a review from xedin October 20, 2018 00:14
@rudkx
Copy link
Contributor Author

rudkx commented Oct 20, 2018

@swift-ci Please smoke test

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

There are couple of comments but otherwise LGTM!

designatedNominalTypes[designatedTypeIndex];

if (parentDecl == designatedNominal) {
auto &constraints = definedInDesignatedType[designatedTypeIndex];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think bodies of both of these ifs could be unified into:

// preamble
auto *parentDC = functionDecl->getParent();
auto *parentDecl = parentDC->getSelfNominalTypeDecl();

// in the body
if (parentDecl == designatedNominal) {
  auto &container = parentDC->isExtensionContext() ? definedInExtensionOfDesignatedType : definedInDesignatedType;
  container[designatedTypeIndex].push_back(constraint);
  return true;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, something like this should work.

}
}
SmallVector<SmallVector<unsigned, 4>, 4> definedInDesignatedType;
SmallVector<SmallVector<unsigned, 4>, 4> definedInExtensionOfDesignatedType;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why would we'd want to pass these into a partitionForDesignatedTypes call instead of just making both local in there because they are not used partitionDisjunction?...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oversight. Will fix momentarily.

@rudkx
Copy link
Contributor Author

rudkx commented Oct 20, 2018

@swift-ci Please smoke test

@rudkx rudkx merged commit 2b091f8 into swiftlang:master Oct 20, 2018
@rudkx rudkx deleted the refactor-disjunction-partitioning branch October 20, 2018 20:49
@atrick
Copy link
Contributor

atrick commented Oct 21, 2018

@rudkx Is this really the reason that Driver/sdk-apple.swift test case is failing now in CI? There aren't any other commits:

https://ci.swift.org/job/oss-swift-incremental-RA-osx/5357/consoleFull#-2082087619ba62d58e-7248-467b-91e0-c7508d5cf947

@rudkx
Copy link
Contributor Author

rudkx commented Oct 21, 2018

@atrick That would be pretty unusual. It looks like another run is happening now so we can see how far that gets. I did a complete -T test run on my machine locally before pushing my changes.

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