Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support user-defined implicit conversions via
ImplicitAs
#1273Support user-defined implicit conversions via
ImplicitAs
#1273Changes from 23 commits
fbf33ae
43ed1fc
294c6f7
f0c8e3d
6f66126
bbf24ab
2aaf688
cb431de
81e9791
e25a1ef
6fb8771
00d3bec
e71e814
8de4baa
d9cba7e
9dc0fa1
995862e
3534f00
566f58b
d142d52
c6349ee
069f527
eee2e57
d2af4ad
26239fc
dcbaf0d
bd6263d
defdf2f
2402ccf
30894b3
a7a6ddd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where you're adding setters, is there anything this can do relative to checking type checking of rhs_ or rhs that would help enforce the "Can only"? Maybe it would make sense to do something like "set_desugared_rhs" which could only be called once, and a desugared_rhs getter that returns desugared_rhs if set, rhs otherwise?
Note, I'm wondering about this mainly in the larger context of adding setters. This could go a step further with a templated type, e.g.:
Then maybe something like this becomes:
However, that's a much higher code delta, especially because it affects interpreter call sites. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is probably a better direction here, but I think it's likely to take a bit of discussion to hammer out the best approach. Do you want this addressed before this change lands, or can this be deferred to a later refactoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's probably reasonable to defer to a later refactoring, that was part of my concern over size of the delta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I'd typically suggest TODO instead of FIXME for anything that's expected to outlast the given PR, because that's what the style guide says to use:
https://google.github.io/styleguide/cppguide.html#TODO_Comments
I haven't commented much on this because I'm not sure how much you're using FIXME to indicate things you intend to fix either before merging or quickly after, but this sounds like it's very much "eventually".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to note, I'm doing more cleanup of this in #1282
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably wrap these to 80 char columns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, though I somewhat arbitrarily invented some wrapping and indent rules. I assume we'll eventually have a tool to automate this, and we'll run it over all of our existing Carbon code once it exists, so I guess the precise formatting choices right now don't matter too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, arbitrary rules is kind of what I expected.