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

And, or, not #680

Merged
merged 8 commits into from
Aug 3, 2021
Merged

And, or, not #680

merged 8 commits into from
Aug 3, 2021

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Jul 27, 2021

Logical AND, OR, and NOT are important building blocks for working with Boolean values. This proposal introduces the and, or, and not operators to support these operations.

@zygoloid zygoloid added the proposal A proposal label Jul 27, 2021
@zygoloid zygoloid requested a review from a team July 27, 2021 01:08
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Jul 27, 2021
@zygoloid zygoloid marked this pull request as ready for review July 27, 2021 02:20
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Really nice... My only comments are trying to make the alternatives section a little less ambiguous/unclear for readers.

proposals/p0680.md Outdated Show resolved Hide resolved
proposals/p0680.md Outdated Show resolved Hide resolved
proposals/p0680.md Outdated Show resolved Hide resolved
proposals/p0680.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

FWIW I'm still mixed on and/or/not. On the one hand, I feel like there are lots of languages that use && and ||, and I feel like Python is kind of on its own here -- I worry that this will make Carbon feel more niche, and that this is a design decision being made due to a single (if major) bug in Chrome, which could possibly be addressed by restricting how bitwise operations work instead (and bitwise operations are rarer, my sense is probably by an order of magnitude). On the other hand, I do sympathize with readability, and maybe Python is enough precedent to make this stand.

var x = cond1 == not cond2;
```

is invalid and requires parentheses, and
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused why the above example on line 197 is ambiguous in a way that would lead to it being invalid, can you clarify this? I would've thought there's no operand other than not touching cond2 that could lead to an alternative interpretation for which precedence would be needed to resolve...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not is lower precedence than ==, so we can't have a not expression as a subexpression of an == expression following the precedence proposal. We could choose to have different rules on the left and right side of ==, so not a == b would mean not (a == b) but b == not a would mean b == (not a), but I think overall that would be too confusing, and it creates problems with things like a == not b == c, which would parse as a == (not (b == c)) using the normal operator precedence parser approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I still don't understand why this fails but not cond1 == cond2 should work (I would have thought that would be the ambiguous case indicative of an error).

Do what you like with this commentary, I don't think this is specific to the keyword spelling. At this point I'm not really asking for changes/replies, just observing that I don't really understand the intended behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I think this is something that will become even less ambiguous with grammar and the design document so not worried about getting it perfect in the proposal.

proposals/p0680.md Show resolved Hide resolved
@jonmeow
Copy link
Contributor

jonmeow commented Jul 27, 2021

I think I should note, using &&/||/! seems crucial to address directly. This had me looking more closely at the rationale, where I think it talks about the advantages of and, but I feel the disadvantages are overlooked.

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

(apparently, pending comments don't always attach to comments)

proposals/p0680.md Outdated Show resolved Hide resolved
@zygoloid zygoloid mentioned this pull request Jul 27, 2021
Copy link
Contributor Author

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

See also #682, which gives an example of what this proposal would look like if applied to the Carbon project's C++ code.

proposals/p0680.md Outdated Show resolved Hide resolved
proposals/p0680.md Outdated Show resolved Hide resolved
proposals/p0680.md Outdated Show resolved Hide resolved
proposals/p0680.md Outdated Show resolved Hide resolved
proposals/p0680.md Outdated Show resolved Hide resolved
var x = cond1 == not cond2;
```

is invalid and requires parentheses, and
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not is lower precedence than ==, so we can't have a not expression as a subexpression of an == expression following the precedence proposal. We could choose to have different rules on the left and right side of ==, so not a == b would mean not (a == b) but b == not a would mean b == (not a), but I think overall that would be too confusing, and it creates problems with things like a == not b == c, which would parse as a == (not (b == c)) using the normal operator precedence parser approach.

proposals/p0680.md Show resolved Hide resolved
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Thanks, I think the text here is converging nicely. Some adjustments based on feedback on this thread (thanks!) and I'm going to create a blocking issue for deciding specifically on the strategy here.

Comment on lines 191 to 195
### Use punctuation spelling for all three operators

We could follow the convention established by C and spell these operators as
`&&`, `||`, and `!`. This would improve familiarity for people comfortable with
this operator set.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this at least should be a blocking issue for the leads. I'll turn this comment into that issue.

I'd also suggest structuring this as advantages / disadvantages? Mostly I think that'll help extract some of the advantages of using && and similar spellings from the prose to make it clear that they do exist and are being considered.

proposals/p0680.md Outdated Show resolved Hide resolved
proposals/p0680.md Outdated Show resolved Hide resolved
proposals/p0680.md Outdated Show resolved Hide resolved
proposals/p0680.md Show resolved Hide resolved
proposals/p0680.md Show resolved Hide resolved
@chandlerc
Copy link
Contributor

Depends on #684

@jonmeow
Copy link
Contributor

jonmeow commented Jul 29, 2021

Apologies, I'm realizing this proposal is still in Draft according to the project. Did you mean to move it to the RFC column?

@josh11b
Copy link
Contributor

josh11b commented Jul 29, 2021

Apologies, I'm realizing this proposal is still in Draft according to the project. Did you mean to move it to the RFC column?

Perhaps the automation should do that automatically once the PR itself is marked as ready for review?

@jonmeow
Copy link
Contributor

jonmeow commented Jul 29, 2021

It does if you switch project columns. It cannot change project columns on its own.

@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Jul 31, 2021
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Blocking decision is resolved, LGTM.

I've left one minor suggested addition below and responded to the only unresolved comment thread I saw. If we need specific clarification, we'll have a place in a follow-up I think.

proposals/p0680.md Outdated Show resolved Hide resolved
var x = cond1 == not cond2;
```

is invalid and requires parentheses, and
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I think this is something that will become even less ambiguous with grammar and the design document so not worried about getting it perfect in the proposal.

@zygoloid zygoloid merged commit fac4343 into carbon-language:trunk Aug 3, 2021
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Aug 3, 2021
jonmeow added a commit that referenced this pull request Jan 27, 2022
Co-authored-by: Chandler Carruth <[email protected]>

Co-authored-by: Richard Smith <[email protected]>
@zygoloid zygoloid deleted the proposal-and-or-not branch March 11, 2022 01:02
chandlerc added a commit that referenced this pull request Jun 28, 2022
Proposal: use `and`, `or`, and `not` keywords in place of `&&`, `||`, `!`.

Co-authored-by: Chandler Carruth <[email protected]>
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Co-authored-by: Chandler Carruth <[email protected]>

Co-authored-by: Richard Smith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants