Cleaned up version of annotation code #414

Closed
wants to merge 21 commits into
from

Conversation

Projects
None yet
3 participants
@endobson
Contributor

endobson commented Aug 29, 2013

Needs the new version of TR unit testing code to correctly work, but the changes are separate.

@endobson

This comment has been minimized.

Show comment Hide comment
@endobson

endobson Oct 20, 2013

Contributor

Ping.

Contributor

endobson commented Oct 20, 2013

Ping.

@endobson

This comment has been minimized.

Show comment Hide comment
@endobson

endobson Dec 2, 2013

Contributor

Ping.

Contributor

endobson commented Dec 2, 2013

Ping.

(cond
[(type-ascription-property stx)
=>
(lambda (prop)
- (let loop ((prop prop))

This comment has been minimized.

Show comment Hide comment
@samth

samth Jan 15, 2014

Member

Why is taking out this loop ok? Are we guaranteed never to merge these properties?

@samth

samth Jan 15, 2014

Member

Why is taking out this loop ok? Are we guaranteed never to merge these properties?

This comment has been minimized.

Show comment Hide comment
@endobson

endobson Jan 16, 2014

Contributor

Yes, because merging the properties happened because two annotations could apply properties to the same syntax object. Now since they are on the #%expression that cannot happen.

@endobson

endobson Jan 16, 2014

Contributor

Yes, because merging the properties happened because two annotations could apply properties to the same syntax object. Now since they are on the #%expression that cannot happen.

+
+
+;; Typecheck an (#%expression e) form
+(define (tc/expression form expected)

This comment has been minimized.

Show comment Hide comment
@samth

samth Jan 15, 2014

Member

I think tc/expression is a confusing name here -- I know we can't use tc/#%expression, but something to indicate the real purpose would be good.

@samth

samth Jan 15, 2014

Member

I think tc/expression is a confusing name here -- I know we can't use tc/#%expression, but something to indicate the real purpose would be good.

This comment has been minimized.

Show comment Hide comment
@takikawa

takikawa Jan 15, 2014

Contributor

Isn't that a valid identifier?

Welcome to Racket v6.0.0.1.
-> (define tc/#%expression 3)
-> tc/#%expression
3
@takikawa

takikawa Jan 15, 2014

Contributor

Isn't that a valid identifier?

Welcome to Racket v6.0.0.1.
-> (define tc/#%expression 3)
-> tc/#%expression
3

This comment has been minimized.

Show comment Hide comment
@samth

samth Jan 15, 2014

Member

Oh, great. I thought it only worked at the beginning.

@samth

samth Jan 15, 2014

Member

Oh, great. I thought it only worked at the beginning.

+ (tc-expr #'e))]))
+
+
+;; do-inst : syntax? tc-results/c -> tc-results/c

This comment has been minimized.

Show comment Hide comment
@samth

samth Jan 15, 2014

Member

This should be (or/c #f syntax?), right?

@samth

samth Jan 15, 2014

Member

This should be (or/c #f syntax?), right?

This comment has been minimized.

Show comment Hide comment
@endobson

endobson Jan 16, 2014

Contributor

I assume you meant (or/c tc-results/c #f), which is correct.

@endobson

endobson Jan 16, 2014

Contributor

I assume you meant (or/c tc-results/c #f), which is correct.

+ (not (= (syntax-length inst) (Poly-n ty))))
+ (tc-error/expr "Wrong number of type arguments to polymorphic type ~a:\nexpected: ~a\ngot: ~a"
+ (cleanup-type ty) (Poly-n ty) (syntax-length inst))]
+ [(and (PolyDots? ty) (not (>= (syntax-length inst) (sub1 (PolyDots-n ty)))))

This comment has been minimized.

Show comment Hide comment
@samth

samth Jan 15, 2014

Member

We could use < here.

@samth

samth Jan 15, 2014

Member

We could use < here.

+ ;; In this case, we need to check the last thing. If it's a dotted var, then we need to
+ ;; use instantiate-poly-dotted, otherwise we do the normal thing.
+ ;; In the case that the list is empty we also do the normal thing
+ (let ((stx-list (syntax->list inst)))

This comment has been minimized.

Show comment Hide comment
@samth

samth Jan 15, 2014

Member

Nit: [].

@samth

samth Jan 15, 2014

Member

Nit: [].

This comment has been minimized.

Show comment Hide comment
@samth

samth Jan 15, 2014

Member

Or internal-define.

@samth

samth Jan 15, 2014

Member

Or internal-define.

+ ;; use instantiate-poly-dotted, otherwise we do the normal thing.
+ ;; In the case that the list is empty we also do the normal thing
+ (let ((stx-list (syntax->list inst)))
+ (if (null? stx-list)

This comment has been minimized.

Show comment Hide comment
@samth

samth Jan 15, 2014

Member

Merge this if into the match below.

@samth

samth Jan 15, 2014

Member

Merge this if into the match below.

@@ -1,40 +0,0 @@
-#lang scheme/base

This comment has been minimized.

Show comment Hide comment
@samth

samth Jan 15, 2014

Member

Why are we dropping these tests?

@samth

samth Jan 15, 2014

Member

Why are we dropping these tests?

This comment has been minimized.

Show comment Hide comment
@endobson

endobson Jan 16, 2014

Contributor

Because they were broken (in terms of phasing) and didn't offer any more coverage than our typecheck tests of similar programs.

@endobson

endobson Jan 16, 2014

Contributor

Because they were broken (in terms of phasing) and didn't offer any more coverage than our typecheck tests of similar programs.

@@ -18,9 +18,6 @@ TR info: dead-inf-comp.rkt 204:4 (>= -inf.f rat) -- exact real arith
TR info: dead-inf-comp.rkt 207:4 (>= rat -inf.f) -- exact real arith
TR info: dead-inf-comp.rkt 212:41 displayln -- hidden parameter
TR info: dead-inf-comp.rkt 212:41 displayln -- hidden parameter
-TR opt: dead-inf-comp.rkt 102:0 #%module-begin -- dead else branch

This comment has been minimized.

Show comment Hide comment
@samth

samth Jan 15, 2014

Member

Why does this change affect the optimizer output?

@samth

samth Jan 15, 2014

Member

Why does this change affect the optimizer output?

This comment has been minimized.

Show comment Hide comment
@endobson

endobson Jan 16, 2014

Contributor

Not sure exactly, but it looks like it is better. The dead else branch was not at the module begin but inside the for loop. My guess is that the #%expression allowed it to find it correctly.

@endobson

endobson Jan 16, 2014

Contributor

Not sure exactly, but it looks like it is better. The dead else branch was not at the module begin but inside the for loop. My guess is that the #%expression allowed it to find it correctly.

@@ -1,7 +1,7 @@
#;#;
#<<END
TR opt: vector-length.rkt 15:0 (vector-length (vector 1 2 3)) -- known-length vector-length
-TR opt: vector-length.rkt 16:0 (vector-length (ann (vector 4 5 6) (Vectorof Integer))) -- known-length vector-length
+TR opt: vector-length.rkt 16:0 (vector-length (ann (vector 4 5 6) (Vectorof Integer))) -- vector-length

This comment has been minimized.

Show comment Hide comment
@samth

samth Jan 15, 2014

Member

Why are there fewer optimizations here?

@samth

samth Jan 15, 2014

Member

Why are there fewer optimizations here?

This comment has been minimized.

Show comment Hide comment
@endobson

endobson Jan 16, 2014

Contributor

Because the optimizer can no longer see through type annotations. The type of the expression is (Vectorof Integer) not (Vector Integer Integer Integer).

@endobson

endobson Jan 16, 2014

Contributor

Because the optimizer can no longer see through type annotations. The type of the expression is (Vectorof Integer) not (Vector Integer Integer Integer).

@endobson

This comment has been minimized.

Show comment Hide comment
@endobson

endobson Jan 16, 2014

Contributor

I believe have addressed all the current comments.

Contributor

endobson commented Jan 16, 2014

I believe have addressed all the current comments.

@endobson

This comment has been minimized.

Show comment Hide comment
@endobson

endobson Feb 22, 2014

Contributor

Is this good to go in with fixups merged?

Contributor

endobson commented Feb 22, 2014

Is this good to go in with fixups merged?

@samth

This comment has been minimized.

Show comment Hide comment
@samth

samth Feb 25, 2014

Member

Yes, r+ for this.

Member

samth commented Feb 25, 2014

Yes, r+ for this.

@samth

This comment has been minimized.

Show comment Hide comment
@samth

samth Mar 31, 2014

Member

@shekari ping?

Member

samth commented Mar 31, 2014

@shekari ping?

@endobson

This comment has been minimized.

Show comment Hide comment
@endobson

endobson Apr 1, 2014

Contributor

As I merged this forward I ran into the match bug with multiple (list a ...) patterns. To solve that I needed to fix check-below. Fixing check below correctly required fixing a bunch of cases that were misusing expected values.

So long story short this is in the process of being merged but it is non trivial due to it bringing up other bugs.

Contributor

endobson commented Apr 1, 2014

As I merged this forward I ran into the match bug with multiple (list a ...) patterns. To solve that I needed to fix check-below. Fixing check below correctly required fixing a bunch of cases that were misusing expected values.

So long story short this is in the process of being merged but it is non trivial due to it bringing up other bugs.

@endobson

This comment has been minimized.

Show comment Hide comment
@endobson

endobson May 7, 2014

Contributor

This is finally all in.

Contributor

endobson commented May 7, 2014

This is finally all in.

@endobson endobson closed this May 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment