-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Let variables #1095
Let variables #1095
Conversation
I have a fix for the failing test where I add the following on line 116 of
Should I add this as a commit to this branch or is that not the proper way to solve this issue? |
I'm not sure; @jonmeow or @chandlerc, how would you recommend dealing with this? |
See my comments in #executable-semantics, but note I'd expect that if this is patched it should probably be because all PRs are failing like this. I don't see any edits to the build system in this PR, which makes me wonder, is this transient and maybe related to a pending change in GH that may not remain? |
(if you haven't already, try re-running the pre-commits to see if it's transient) |
n/m. See #build-system comments. |
Should be able to merge from trunk now to get the latest llvm. |
@jonmeow I guess that error was transient because it's fixed now 😅 |
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.
That's what I suspected -- maybe GH was modifying their action docker instances and forgot a terminal package. But, that could end up sticking eventually, and I know you had issues, which is why I've also pushed through the llvm-project fix (although per discussion we still need to do more there).
Anyways, I'm dropping in a couple capitalization/punctuation fixes in comments while I'm here, but assuming geoffromer's still the primary review.
Made some changes to the interpretation of nested |
// A pattern that matches a value of a specified type, and optionally binds | ||
// a name to it. | ||
class BindingPattern : public Pattern { | ||
public: | ||
using ImplementsCarbonValueNode = void; | ||
|
||
BindingPattern(SourceLocation source_loc, std::string name, | ||
Nonnull<Pattern*> type) | ||
Nonnull<Pattern*> type, | ||
std::optional<ValueCategory> value_category) |
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.
Non-blocking observation: I'd prefer if we could always get this from the typechecker, rather than sometimes as a constructor parameter, but I think this is a consequence of some pre-existing problems that are out of scope for this PR.
* Modify parser and AST nodes to include let statement * Implement let variables * Remove redundant code in fail_match_choice test * Add comment for has_value_category() * Apply suggestions from code review Co-authored-by: Jon Meow <[email protected]> * clang-format changes * Update comments for new BindingPattern methods * Implement nested vars in patterns * Apply suggestions from @geoffromer's code review Co-authored-by: Geoff Romer <[email protected]> * Implement changes from code review. * Remove maybe_var_pattern grammar rule Co-authored-by: Jon Meow <[email protected]> Co-authored-by: Geoff Romer <[email protected]>
Implement let/const variables as specified in #821