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

feat(compiler): Convert runtime/dataStructures.gr to primitives #1145

Merged
merged 4 commits into from
Mar 6, 2022

Conversation

ospencer
Copy link
Member

This means that all of the data structures functions get inlined, so there's no performance cost to using something like untagNumber.

Copy link
Member

@marcusroberts marcusroberts left a comment

Choose a reason for hiding this comment

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

From my limited understanding looks good. I see you've updated the formatter, the new LSP may need updating when merged too, maybe not if oprint is doing the work here.

Copy link
Member

@peblair peblair left a comment

Choose a reason for hiding this comment

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

I love it.

67kced

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Since you are doing a lot of work in dataStructures, can you please make sure the graindoc is correct throughout?

Types.StackAllocated(WasmI32),
Types.StackAllocated(WasmI32),
],
[Types.HeapAllocated, Types.HeapAllocated, Types.HeapAllocated],
Copy link
Member

Choose a reason for hiding this comment

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

Why is this changed to heap allocated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was just wrong before. This is a little bit of a subtle bug to catch too, since it'd only be a problem if you were pattern matching on heap-allocated constants. I only noticed it because of the refactor.

*
* @returns {WasmI32}
* @returns
Copy link
Member

Choose a reason for hiding this comment

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

Gave up on writing these?

stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
*
* @returns {WasmI32}
* @returns
Copy link
Member

Choose a reason for hiding this comment

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

Gave up on writing these?

stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
ptr
}
@unsafe
export primitive allocateRational: () -> WasmI32 = "@allocate.rational"

/**
* Allocates a new Rational with a prepopulated value
* @param value The value to store
Copy link
Member

Choose a reason for hiding this comment

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

Wrong docs? Can you please update

* @param wrappedRational The boxed rational to return
* Load the (tagged) variant of an ADT.
*
* @export
Copy link
Member

Choose a reason for hiding this comment

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

wat

* @param wrappedRational The boxed rational to return
* Load an untagged string's size.
*
* @export
Copy link
Member

Choose a reason for hiding this comment

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

wat?


/**
* Load a value from an ADT.
* Load an untagged Bytes' size.
*
* @export
Copy link
Member

Choose a reason for hiding this comment

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

wat?


/**
* Load an untagged string's size.
* Untag a simple number.
*
* @export
Copy link
Member

Choose a reason for hiding this comment

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

wat?

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Slight mistake on the graindocs. Can you generate the markdown file for this file so we can know that everything is doc'd correctly?

stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
stdlib/runtime/dataStructures.gr Outdated Show resolved Hide resolved
@ospencer
Copy link
Member Author

ospencer commented Mar 6, 2022

@phated Let me know if that looks good.

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Good enough! I had some stylistic nits but not worth it for this internal documentation.

@ospencer ospencer force-pushed the oscar/unsafe-number branch 3 times, most recently from bad2609 to 6c2d1d9 Compare March 6, 2022 15:56
@ospencer ospencer force-pushed the oscar/data-structures-primitives branch from 3f278be to 66c0bab Compare March 6, 2022 17:23
Base automatically changed from oscar/unsafe-number to main March 6, 2022 18:26
@ospencer ospencer force-pushed the oscar/data-structures-primitives branch from 66c0bab to 03f8232 Compare March 6, 2022 18:27
@ospencer ospencer merged commit 2d43b28 into main Mar 6, 2022
@ospencer ospencer deleted the oscar/data-structures-primitives branch March 6, 2022 18:46
@github-actions github-actions bot mentioned this pull request May 16, 2022
@github-actions github-actions bot mentioned this pull request May 31, 2022
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.

4 participants