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

Adding an intrinsics module #1573

Merged
merged 12 commits into from
May 18, 2022
Merged

Adding an intrinsics module #1573

merged 12 commits into from
May 18, 2022

Conversation

nfurfaro
Copy link
Contributor

All in the title.

close #1532

@nfurfaro nfurfaro added the lib: std Standard library label May 17, 2022
@nfurfaro nfurfaro self-assigned this May 17, 2022
Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Does this actually work?? I'd like to see some tests.

sway-lib-std/src/intrinsics.sw Outdated Show resolved Hide resolved
sway-lib-std/src/intrinsics.sw Outdated Show resolved Hide resolved
@adlerjohn adlerjohn marked this pull request as draft May 17, 2022 18:07
@adlerjohn
Copy link
Contributor

Converting to draft until tested and CI passes.

@nfurfaro nfurfaro marked this pull request as ready for review May 17, 2022 19:17
@nfurfaro nfurfaro requested a review from adlerjohn May 17, 2022 19:17
Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Remove the use of value since that's not how is_reference_type() and size_of() are used.

Comment on lines +11 to +17
fn is_ref_type<T>(param: T) -> bool {
is_reference_type::<T>()
}

fn get_size_of<T>(param: T) -> u64 {
size_of::<T>()
}
Copy link
Contributor

@otrho otrho May 17, 2022

Choose a reason for hiding this comment

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

I wonder if the actual library functions in the intrinsics module should do this? They seem a bit redundant otherwise. What is the motivation around creating the wrappers in the new module -- to hide the __ prefix?

I guess most intrinsics, like those which will wrap VM opcodes, are not really expected to be exposed to the average dev. They're more for low level library development, and we wouldn't need/want to wrap them. But these are designed for everyday use so sure, let's wrap. But I don't anticipate needing to put anything else in this module particularly..? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

to hide the __ prefix?

Yes, because the alternative is to make the specific names of the intrinsics part of the public API of the language. Which we could certainly do, but then it's one more set of keywords people have to account for.

But I don't anticipate needing to put anything else in this module particularly..?

Can't really think of anything either, but that's fine! Not every intrinsic needs to be available to the end user, as you said.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in favor of these wrappers to hide the __ prefix. It helps keep a nice uniform api surface for users.

@adlerjohn adlerjohn requested a review from otrho May 18, 2022 13:39
@mohammadfawaz mohammadfawaz mentioned this pull request May 18, 2022
8 tasks
@nfurfaro nfurfaro merged commit 438a93d into master May 18, 2022
@nfurfaro nfurfaro deleted the furnic/1532-intrinsics-module branch May 18, 2022 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib: std Standard library
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Include an intrinsics module in the standard library that simply wraps existing intrinsics.
4 participants