"c || a && b" should be true for a,c about bool HOT 26 CLOSED

The unit tests that demonstrate the bug are on the broken-boolean-logic branch. It fails on both Ruby/JRuby, and if we were to add similar tests for Java and JavaScript (we should) I assume they would fail in a similar way.

from bool.

Cool!

My guess is this is about precedence. We need to tell it we mean c || (a && b)

I was wondering if we needed to do this, and whether it would help us to explore a bit more about how gherkin will have to work, in terms of having a logical structure on top of the parsed tree. Glad you've found an example!

from bool.

AND and OR have been given equal precedence in parser.y by the statement:

%left TOKEN_OR TOKEN_AND

Splitting it into two lines gets the test to pass:
%left TOKEN_OR
%left TOKEN_AND

We'll need to consider the precedence of TOKEN_NOT too, I suppose. Also, parens?

More examples required.

On 15/02/2013, Matt Wynne [email protected] wrote:

Cool!

My guess is this is about precedence. We need to tell it we mean c || (a && b)

I was wondering if we needed to do this, and whether it would help us to
explore a bit more about how gherkin will have to work, in terms of having a
logical structure on top of the parsed tree. Glad you've found an example!

---

Reply to this email directly or view it on GitHub:
#32 (comment)

from bool.

@sebrose good catch!

One way to test that the parser works correctly is to represent parsed expressions with explicit parenthesis:

Example:

def explicit
  ast.describe_to(Bool::Explicit.new, "")
end

let(:expression) { "c || a && b" }

it "expands properly" do
  explicit.must_equal("(c || (a && b))")
end

This can be done easily with a new Explicit visitor that returns a string. This way we can test parsing of many different expressions without even having to evaluate them.

from bool.

On 15 February 2013 10:44, Aslak Hellesøy [email protected] wrote:

@sebrose https://github.com/sebrose good catch!

One way to test that the parser works correctly is to represent parsed
expressions with explicit parenthesis:

Example:

def explicit
ast.describe_to(Bool::Explicit.new, "")end
let(:expression) { "c || a && b" }
it "expands properly" do
explicit.must_equal("(c || (a && b))")end

This can be done easily with a new Explicit visitor that returns a string.
This way we can test parsing of many different expressions without even
having to evaluate them.

Nice.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/32#issuecomment-13601220.

ACCU - Professionalism in Programming - http://accu.org

http://www.claysnow.co.uk
http://twitter.com/#!/sebrose
http://uk.linkedin.com/in/sebrose

from bool.

In the Java tests the following were my results:
a || b && c given a, b should be true failed
a || b && c given a, c should be true passed!!

Strange.. as the above comments say that the second one failed in Ruby.

Unless I have mistyped something and my test is buggy ...

...its strange that Ruby failed on one that passed Java.

But Java is still failing on a different case; so at least it still fails. Is Ruby passing on the one that failed for Java?

from bool.

@ilanpillemer your a || b && c given a, c looks nothing like my original test: c || a && b given a, c

Thanks for writing java tests, but what about my suggestion to render the AST with parentheses instead?

from bool.

On Sunday, February 17, 2013, Aslak Hellesøy wrote:

@ilanpillemer https://github.com/ilanpillemer your a || b && c given a,
c looks nothing like my original test: c || a && b given a, c

oops.. my bad. I now see its exactly like the one that fails.

Thanks for writing java tests, but what about my suggestion to render the
AST with parentheses instead?

I will try look at doing that tonight. Just wanted to get a failing test
first.

Ilan

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/32#issuecomment-13682173.

from bool.

Now that the Explicit Visitor is working nicely and tests are passing... should we delete the EvaluatorTest?
Since by simply rendering the expression with parentheses as suggested by @aslakhellesoy , the tests run so much faster.. and since the Evaluator test is really slow - Is there any value now in the test being there?

from bool.

On 17 Feb 2013, at 18:32, Ilan Pillemer [email protected] wrote:

Now that the Explicit Visitor is working nicely and tests are passing... should we delete the EvaluatorTest?

Are we going to delete the Evaluator too, or just the tests?

cheers,
Matt

http://mattwynne.net || https://twitter.com/mattwynne || http://the-cucumber-book.com || http://bddkickstart.com || http://www.relishapp.com

from bool.

Just the tests that are now being served by the ExplicitTests.
But I think at least one basic test should remain for the Evaluator. I am just in a dilemma about how basic it should be.

from bool.

edited parser.y so that all ruby and jruby tests pass.
344e1de

from bool.

So I have created an explicit visitor in Java, Javascript and Ruby. Added tests, and then changed the C, javascript and Java parser definition files to make the tests pass.

I wanted to change at least the Java Class names for Evaluator to EvaluatorVisitor but that broke JRuby. This would then mean I would have to change the Ruby class names as well. I did not feel comfortable doing that without agreement so I changed the Java Class names back to Evaluator and Explicit.

How do others feel about having Visitor as a suffix in Visitor implementations?

I think this is ready to be merged back into Master?

from bool.

I have noticed that the make fails after a clean; due to warnings from the C compiler. This also happens on master... which makes me wonder why Master is not failing to build.. does Travis run clean every time?

from bool.

Specifically the make fails after a clean with

cc -fPIC -O2 -Werror -Wall -Wextra   -c -o parser.o parser.c
cc -fPIC -O2 -Werror -Wall -Wextra   -c -o lexer.o lexer.c
lexer.c:784:23: error: comparison of integers of different signs: 'yy_size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
                        for ( yyl = 0; yyl < (int)yyleng; ++yyl )
                                       ~~~ ^ ~~~~~~~~~~~
lexer.c:1610:17: error: comparison of integers of different signs: 'yy_size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
        for ( i = 0; i < (int)_yybytes_len; ++i )
                     ~ ^ ~~~~~~~~~~~~~~~~~
2 errors generated.
make[1]: *** [lexer.o] Error 1
make: *** [c] Error 2

But if you compile it first with warnings off; then make works fine from then on; until another make clean.

As mentioned this is separate from the issue this branch is for; so should we merge this anyway?

from bool.

Which compiler? What version of the compiler?

On 19/02/2013, Ilan Pillemer [email protected] wrote:

Specifically the make fails after a clean with

cc -fPIC -O2 -Werror -Wall -Wextra   -c -o parser.o parser.c
cc -fPIC -O2 -Werror -Wall -Wextra   -c -o lexer.o lexer.c
lexer.c:784:23: error: comparison of integers of different signs:
'yy_size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
                        for ( yyl = 0; yyl < (int)yyleng; ++yyl )
                                       ~~~ ^ ~~~~~~~~~~~
lexer.c:1610:17: error: comparison of integers of different signs:
'yy_size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
        for ( i = 0; i < (int)_yybytes_len; ++i )
                     ~ ^ ~~~~~~~~~~~~~~~~~
2 errors generated.
make[1]: *** [lexer.o] Error 1
make: *** [c] Error 2

But if you compile it first with warnings off; then make works fine from
then on; until another make clean.

As mentioned this is separate from the issue this branch is for; so should
we merge this anyway?

---

Reply to this email directly or view it on GitHub:
#32 (comment)

from bool.

On 18 Feb 2013, at 21:10, Ilan Pillemer [email protected] wrote:

So I have created an explicit visitor in Java, Javascript and Ruby. Added tests, and then changed the C, javascript and Java parser definition files to make the tests pass.

I wanted to change at least the Java Class names for Evaluator to EvaluatorVisitor but that broken JRuby. This would then mean I would have to change the Ruby class names as well. I did not feel comfortable doing that without agreement so I changed the Java Class names back to Evaluator and Explicit.

How do others feel about having Visitor as a suffix in Visitor implementations?

I specifically removed that suffix, the details of why are in #12

As I said in that ticket, I prefer trying to find domain-meaningful names for classes whenever possible, rather than leaning on design patterns to name things. If my understanding is correct, I'd say the Explicit class might be better named ExplicitRenderer or even just Renderer, since it's job is to render the expression tree into a string.

from bool.

I guess its just a personal preference. When dealing with projects with many classes I like the self-documenting nature of using suffixes - as I can see at a glance which classes are Visitors, which are Factories etc. But it makes no real difference when there are only a handful of classes. I just had to open up the java source file of every class to see what it was - rather than just know from the class name. If there were hundreds of classes this would annoy me.

from bool.

Also describeTo should be accept. I see value in keeping with convention as it allows people familiar with the visitor design pattern to understand the code much quicker.

I think the Visitor Suffix should be added back and describeTo should be accept.

from bool.

@sebrose

Ilans-MacBook-Air:bool ilanpillemer$ cc --version
Apple LLVM version 4.2 (clang-425.0.24) (based on LLVM 3.2svn)
Target: x86_64-apple-darwin12.2.0
Thread model: posix

from bool.

On 20 Feb 2013, at 11:33, Ilan Pillemer [email protected] wrote:

Also describeTo should be accept. I see value in keeping with convention as it allows people familiar with the visitor design pattern to understand the code much quicker.

I think the Visitor Suffix should be added back and describeTo should be accept.

I disagree. Did you read what I wrote #12? I think we should continue this conversation in that ticket.

from bool.

@mattwynne Ok. I have thought about it a lot now... and in the end, I agree with you.
But I am not happy with the name Explicit and with the method describeTo.
I agree with you that Renderer is a better name for this visitor.
I would like to change describeTo to walkWith.

What do you think?

from bool.

And I think the visit methods should, at least in Java, all be renamed walk.

from bool.

And the Visitor interface interface itself should be renamed Walker.

@mattwynne does this align with what you are proposing in terms of domain oriented naming?
Thoughts?

from bool.

@ilanpillemer we're definitely thinking along the same lines now, yeah! However I think we're derailing this particular ticket by talking about this in detail here. Let's create another pull request to do/discuss the further renaming.

from bool.

@mattwynne will do. And I will create a pull request right now for the tests and fix for this issue.
The pull request is created in #33

from bool.