-
Notifications
You must be signed in to change notification settings - Fork 67
Add basic support for vectors #65
base: master
Are you sure you want to change the base?
Conversation
Seems to be about a 4% improvement in compile time.
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.
looks great, just a few minor comments; probably worth it to fix collect() and remove vestigial run-time code when landing
| case TAGS.string: | ||
| return this.schemeToString(ptr); | ||
| case TAGS.symbol: | ||
| if (this.carOf(ptr) != CONSTANTS.null) { |
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 believe the logic is that if the car is a list of chars instead of a string, then it's a gensym. In a new world where strings can be retagged as vectors, then I think the right thing would be to check if it's a vector instead.
| x = this.cdrOf(x); | ||
| if (extract_tag(this.carOf(x)) == TAGS.fixnum) { | ||
| // this is a new style string. | ||
| const length = this.jsFromScheme(this.carOf(x)); |
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 this is from the string PR but it looks good. Might as well include a commit changing to remove the old-string case.
| case TAGS.character: return ptr; | ||
| case TAGS.string: { | ||
| if (extract_tag(from_space[extract_value(ptr) * 2]) == TAGS.fixnum) { | ||
| // new style string |
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.
Also needs follow-up to remove old case. This case should change to be TAGS.vector though, AFAIU.
| const schism = await engine.loadWasmModule(await compiler_bytes()); | ||
| engine.setCurrentInputPort(bytes); | ||
| // let module_package = schism.exports['compile-stdin->module-package'](); | ||
| // module_package = engine.collect(module_package); |
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 we need to re-enable the collect() call, otherwise it's not called.
| (%set-tag (+ allocation 1) tag)))) | ||
| (define (make-vector n init) | ||
| ;; Allocate n + 1 words. %alloc will automatically round up | ||
| ;; if needed. |
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.
Comment probably unneeded; YMMV though :)
| (if (zero? n) | ||
| '() | ||
| (cons '() (make-symbol-table (- n 1))))) | ||
| (define (init-symbol-table table i) |
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.
This function is now unneeded
| (if (zero? (%symbol-table)) | ||
| (begin | ||
| (set-cdr! (%base-pair) (make-symbol-table ,(symbol-table-width))) | ||
| (set-cdr! (%base-pair) (init-symbol-table (make-vector ,(symbol-table-width) '()) 0)) |
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.
The make-vector will handle initialization; can elide call to init-symbol-table
| ;; limitations under the License. | ||
|
|
||
| (library | ||
| (vector-length) |
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.
vector-make
| (define (do-test) | ||
| (let ((v (make-vector 3 '()))) | ||
| (begin | ||
| (vector-set! v 0 5) |
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.
Might be good to test that the initial values of the fields is indeed '()
This is largely derived from #55. This gives basic support for vectors.
This includes
make-vector,vector-set!, andvector-ref. We only support the two argument form ofmake-vector, which takes an initializer to fill all of the elements of the vector with. This ensures we don't have uninitialized memory sitting around.In addition, this replaces the symbol table with a vector instead of a list.
This PR includes #64.