Add basic support for vectors#65
Conversation
Seems to be about a 4% improvement in compile time.
wingo
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
The make-vector will handle initialization; can elide call to init-symbol-table
| ;; limitations under the License. | ||
|
|
||
| (library | ||
| (vector-length) |
| (define (do-test) | ||
| (let ((v (make-vector 3 '()))) | ||
| (begin | ||
| (vector-set! v 0 5) |
There was a problem hiding this comment.
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.