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

Fix not without dependencies #613

Merged
merged 5 commits into from Nov 30, 2016

Conversation

Projects
None yet
2 participants
@ibdknox
Contributor

ibdknox commented Nov 30, 2016

Currently nots that have no dependencies never get an opportunity to run since they are never solving for a variable. This PR adds a prejoin flag to accept and changes NotScan to force an accept check in the case when prejoin is true and the scan has no args. It also adds an extra step to the preJoinAccept function to check nots if they would otherwise not be.

@ibdknox

This comment has been minimized.

Contributor

ibdknox commented Nov 30, 2016

This fixes the issue reported on the mailing list here.

// variable that this scan cares about.
if((!prejoin || this.args.length)
&& (!force && !this.internalVars[solvingFor.id] && this.internalVars.length)
|| !fullyResolved(this.args, prefix)) return true;

This comment has been minimized.

@joshuafcole

joshuafcole Nov 30, 2016

Contributor

I find this conditional to be pretty hard to parse due to its density. It might be worth commenting about the other early acceptance cases or trying to refactor it to split it up.

This is subjective so I don't know that it's blocking.

// if we haven't presolved anything, we still need to do a single prejoin pass
// to make sure that any nots that may have no external dependencies are
// given a chance to end this evaluation
if(presolved === 0) {

This comment has been minimized.

@joshuafcole

joshuafcole Nov 30, 2016

Contributor

Even if variables have been presolved, doesn't this pass need to happen? Or is it invoked when those variables are solved? I assumed presolved variables were the set of variables known prior to solving.

This comment has been minimized.

@ibdknox

ibdknox Nov 30, 2016

Contributor

the loop above will call our not guy with prejoin = true in those cases, so it's handled there. We could just always do this pass and not send the prejoin flag in the loop above. Maybe I'll do that so the not doesn't get re-evaluated over and over again.

// given a chance to end this evaluation
if(presolved === 0) {
for(let provider of providers) {
if(provider instanceof NotScan && !provider.accept(multiIndex, prefix, {id: "0"}, false, true)) {

This comment has been minimized.

@joshuafcole

joshuafcole Nov 30, 2016

Contributor

what is the shape of solvingFor exactly? Why is an object allocation required for each provider?

This comment has been minimized.

@ibdknox

ibdknox Nov 30, 2016

Contributor

It's a variable, I can lift the creation.

add some comments, always check nots in prejoin
Signed-off-by: Chris Granger <ibdknox@gmail.com>
// nots won't get evaluated during Generic Join since we're never solving for a
// variable that this scan cares about.
if((!prejoin || this.args.length)
// if we are forcing and not solving for the current variable, then we just accept

This comment has been minimized.

@joshuafcole

joshuafcole Nov 30, 2016

Contributor

if we aren't forcing... we just accept as it is, right?

@joshuafcole

This comment has been minimized.

Contributor

joshuafcole commented Nov 30, 2016

🔢 🥚 🇪🇬

@ibdknox ibdknox merged commit b0f8f6f into master Nov 30, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@ibdknox ibdknox deleted the fix-not-without-dependencies branch Nov 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment