Skip to content

Remove implicit conjunction#16

Open
dpino wants to merge 1 commit intomasterfrom
issue-16-remove-implicit-conjuction
Open

Remove implicit conjunction#16
dpino wants to merge 1 commit intomasterfrom
issue-16-remove-implicit-conjuction

Conversation

@dpino
Copy link
Member

@dpino dpino commented Aug 14, 2014

I thought that since "tcp port 80" should parse as "tcp and port 80" that in fact the implicit "and" that the pflang spec mentions as obsolete was in fact current. This faulty conclusion was based on my bad understanding of how "tcp port 80" parses -- that in fact as issue #15 indicates, "tcp port 80" is one operator.

So, we should remove the implicit conjunctions from the parser.

dpino added a commit that referenced this pull request Aug 14, 2014
This patch fixes issues #16.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an incorrect change because it will leave prec as nil. dunno what the right fix is; you might try to identify the patch that introduced this behavior and see how to revert it.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I thought it was mostly fixed due to #15. The point is that now when running "make check" the else branch is never executed, so that means for all the tested expressions 'prec' is never nil, but even if that case the patch should have removed the else branch and the check.

So would it still possible that there will be an expression for which 'prec' is nil? I will try to research more on this.

This patch fixes issues #16.
@dpino dpino force-pushed the issue-16-remove-implicit-conjuction branch from 45c61f7 to 5940c73 Compare September 3, 2014 18:16
@kbara
Copy link
Contributor

kbara commented Dec 17, 2014

This is marked as high-priority, but hasn't been closed or merged despite being open for months. Is the underlying issue fixed, or does this patch need to be updated and applied?

@dpino
Copy link
Member

dpino commented Dec 17, 2014

@andywingo commented the proposed patch (remove the else branch) is not correct. I think that once the issue with parsing tcp port was fixed the else branch, that creates the implicit 'and', is no longer taken.

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