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

Pattern matching 3 #45

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

Pattern matching 3 #45

wants to merge 16 commits into from

Conversation

OlasubomiEfuniyi
Copy link
Contributor

Added pattern matching for strings, symbols, and floats

(seq (Cmp rax (imm->bits l))
[(Lit i)
(seq
(Mov r8 (imm->bits i))
Copy link
Member

Choose a reason for hiding this comment

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

Seems needless to go through a register here.

(Push rax)
(compile-string s)
(Push rax)
(compile-string/symbol-eq type-string true false (cons #f (cons #f c)))
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised the code for strings and symbols is so similar. Symbols should be a pointer comparison. Strings need to traverse the string for structural equality. (Although it may be useful to do a quick pointer equality test and fall back on the structural one when the pointers are unequal.)

@@ -488,7 +488,7 @@
(compile-match-clauses cs return c)
(Label return))))

;; [Listof Clauses] Symbol CEnv -> Asm
;;[Listof Clauses] Symbol CEnv -> Asm
Copy link
Member

Choose a reason for hiding this comment

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

Please add back to the space at the front so this doesn't show up in the diff.

@@ -498,7 +498,7 @@
(Label next)
(compile-match-clauses cs return c)))]))

;; Clause Symbol Symbol CEnv -> Asm
;;Clause Symbol Symbol CEnv -> Asm
Copy link
Member

Choose a reason for hiding this comment

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

Add space at front back.

@@ -538,6 +582,40 @@
(Add rsp 16)
(Jmp return))])]))

;;Symbol Symbol CEnv -> ASM
Copy link
Member

Choose a reason for hiding this comment

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

This isn't how you should compare symbols.

@@ -100,7 +100,19 @@
[(Wild) (interp-env e r ds)]
[(Var x) (interp-env e (ext r x v) ds)]
[(Lit l)
(if (eq? l v)
(if (and (number? l) (number? v))
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 it would be better to expand out a mach on l so that you have code specific to each kind of literal, i.e. numbers, should use =, everything else (I think) should use eq?.

@@ -123,7 +135,7 @@

;; Defns Symbol -> Defn
(define (defns-lookup ds f)
(findf (match-lambda [(Defn g _ _) (eq? f g)])
(findf (match-lambda [(Defn g _ _) (equal? f g)])
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a search/replace bug. It should remain eq? unless I'm missing something.

@@ -24,7 +24,7 @@ void error_exit() {
}

void raise_error() {
return error_handler();
return error_handler();
Copy link
Member

Choose a reason for hiding this comment

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

Please remove space so this doesn't show up in the diff.

@@ -12,8 +12,8 @@
;; | (Int Integer)
;; | (Bool Boolean)
;; | (Char Character)
;; | (String)
;; | (Symbol Symbol)
;; | (String s)
Copy link
Member

Choose a reason for hiding this comment

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

s should be String here and Symbol below. (It's a little weird since the name of the constructor and the type of the argument are the same, but saying s doesn't make sense.)

;; | (Cons Id Id)
;; | (Box Id)
;; type Litral = Boolean | '() | Char | Integer
;; type Litral = Boolean | '() | Char | Integer | Symbol | String | Float
Copy link
Member

Choose a reason for hiding this comment

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

Symbol and String should not be in this list.

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