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

Get rid of BoolConst #6

Merged
merged 4 commits into from
Jan 13, 2018
Merged

Get rid of BoolConst #6

merged 4 commits into from
Jan 13, 2018

Conversation

adamjstewart
Copy link
Owner

The purpose of this PR is to remove BoolConst from the AST. This allows for a uniform treatment of "true" and "false" in parsing and evaluating. The following expressions:

cat.
cat :- true.

should have the same representation in the AST now.

@ZaidQureshi
Copy link
Collaborator

I know I sort of made a big deal out of this last night, but I was thinking and I don't think we need to do this as long as we can justify the way we do it in the master branch. The justification that we have for what we have in the master branch is that by having the two ways represented slightly differently we can evaluate the following stuff the same way swi-prolog does it.
db:
a(true).
b(X) :- a(X).

?- b(a(true)).
false.

With making them both the same thing, the evaluator would evaluate that to true.

Also, it is going to be a pain to fix all the code and tests and I would rather focus on the report.

@ZaidQureshi
Copy link
Collaborator

Fixed all evaluator and all tests to remove boolconst. although since you made the pr request you should have fixed all the code, like i did for the previous pr request but its ok.

@adamjstewart
Copy link
Owner Author

When I run make test I see the following warning message:

File "src/evaluator.ml", line 250, characters 34-63:
Warning 11: this match case is unused.

It looks like this block is duplicated. Or should one of them be for false?

@ZaidQureshi
Copy link
Collaborator

fixed

@coveralls
Copy link

coveralls commented Jan 13, 2018

Coverage Status

Coverage increased (+0.09%) to 92.739% when pulling 08e7a66 on features/bool into 1d58fa3 on master.

@coveralls
Copy link

coveralls commented Jan 13, 2018

Coverage Status

Coverage decreased (-0.04%) to 92.612% when pulling 08e7a66 on features/bool into 1d58fa3 on master.

@ZaidQureshi ZaidQureshi merged commit 7d164fe into master Jan 13, 2018
@ZaidQureshi ZaidQureshi deleted the features/bool branch January 13, 2018 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants