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

Implement match condition #861

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

fpottbaecker
Copy link
Collaborator

Closes: #659

Added optional condtions to match cases in RCST, AST, and HIR, which are resolved to if/else in MIR.

Checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

@jwbot jwbot enabled auto-merge January 6, 2024 23:12
@jwbot jwbot added P: Compiler: Language Server Package: The Candy Language Server P: Compiler: Frontend Package: The compiler frontend P: Compiler: Formatter Package: Candy's formatter P: Examples Package: Examples labels Jan 6, 2024
@jwbot
Copy link
Collaborator

jwbot commented Jan 6, 2024

The golden IRs have changed: a68dbb9..3e506fd

@@ -744,6 +744,7 @@ pub fn format_cst<'a>(
}
CstKind::MatchCase {
pattern,
condition: _, // TODO: format match case conditions
Copy link
Member

Choose a reason for hiding this comment

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

I opened a follow-up issue: #868

&mut self,
hir_id: &hir::Id,
condition: Id,
else_builder: E,
Copy link
Member

Choose a reason for hiding this comment

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

You can inline the constraint so there are no type parameters anymore.

Suggested change
else_builder: E,
else_builder: impl FnOnce(&mut Self),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I also apply this to the rest of this class (I was matching the style of other methods in this class)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe later?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should eventually use it where applicable, but that can be done over time. I think the feature is relatively recent, so we only used it for new code and didn't update everything yet

@@ -17,7 +17,7 @@ impl From<CstKind<()>> for Cst<()> {

impl ToRichIr for Rcst {
fn build_rich_ir(&self, builder: &mut RichIrBuilder) {
builder.push(format!("{self:?}"), None, EnumSet::empty());
builder.push(format!("{self:#?}"), None, EnumSet::empty());
Copy link
Member

Choose a reason for hiding this comment

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

The CST's rich IR is currently messed up and I suspect it comes from this change

candy-lang/golden-irs@a68dbb9b012ede17ab5d72dc6cc793aef4cf42e0_..3e506fd0a8da852079633eb18f932fe0baaeca27_#diff-ff8b15b562

let (input, condition) = if let Some((input, condition_comma)) = comma(input) {
let (input, whitespace) = whitespaces_and_newlines(input, indentation, true);
let condition_comma = condition_comma.wrap_in_whitespace(whitespace);
if let Some((input, condition_expresion)) = expression(
Copy link
Member

Choose a reason for hiding this comment

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

Small typo: condition_expresion is missing one “s”

Copy link
Member

Choose a reason for hiding this comment

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

If you completely addressed a review comment and didn't write an answer, you can directly resolve the conversation yourself

Comment on lines 360 to 367
// something %
// foo -> ...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// something %
// foo -> ...
// foo = …
//
// or:
//
// something %
// foo -> ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to further up in this file

compiler/frontend/src/mir/body.rs Outdated Show resolved Hide resolved
@fpottbaecker fpottbaecker force-pushed the 659-implement-match-condition branch from 7f46941 to 38f30f4 Compare March 28, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P: Compiler: Formatter Package: Candy's formatter P: Compiler: Frontend Package: The compiler frontend P: Compiler: Language Server Package: The Candy Language Server P: Examples Package: Examples
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Implement match condition
4 participants