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

Operator precedence 2 #15

Merged
merged 3 commits into from
Jul 20, 2017

Conversation

Normand-1024
Copy link
Contributor

@Normand-1024 Normand-1024 commented Jul 19, 2017

Several changes that I think you should know:

[:compare :compare] cases such as a < b > 1 is now considered not-confusing, mostly due to the false positive case that you sent me. I don't really know any way around it other than this. Plus I think [:compare :compare] has a mixtures of confusing (a <= b != 2) and not confusing (a == b != 1) combinations in it. So I don't feel too bad underclassifying this.

Any confusing combination involving ternary operator will now only concerns itself with the first operand. So a && b ? c : d is considered confusing while a ? b && c : d is not.

Copy link
Owner

@dgopstein dgopstein left a comment

Choose a reason for hiding this comment

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

  1. Please uncommit src/test/resources/i386.c
  2. Please update the comment on that one function that I don't understand (there are also a couple other typos if you want to fix those, but its up to you)

Otherwise looks really good


(defn map-group-pair
"map the group-pair based on the type of node"
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand what this function does. Can you add some more description to the docstring?

Copy link
Owner

Choose a reason for hiding this comment

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

Or maybe a better name too? I'm not really sure.

@dgopstein
Copy link
Owner

Perfect! Thank you.

@dgopstein dgopstein merged commit d9390ca into dgopstein:master Jul 20, 2017
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.

2 participants