Skip to content
This repository was archived by the owner on Aug 31, 2021. It is now read-only.

Conversation

@eholk
Copy link
Contributor

@eholk eholk commented Jul 13, 2019

This is largely derived from #55. This gives basic support for vectors.

This includes make-vector, vector-set!, and vector-ref. We only support the two argument form of make-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.

Copy link
Collaborator

@wingo wingo left a 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) {
Copy link
Collaborator

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));
Copy link
Collaborator

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
Copy link
Collaborator

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);
Copy link
Collaborator

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.
Copy link
Collaborator

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)
Copy link
Collaborator

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))
Copy link
Collaborator

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)
Copy link
Collaborator

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)
Copy link
Collaborator

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 '()

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants