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

endpoint proc-macro #183

Merged
merged 65 commits into from Nov 3, 2020
Merged

endpoint proc-macro #183

merged 65 commits into from Nov 3, 2020

Conversation

YaronWittenstein
Copy link
Contributor

@YaronWittenstein YaronWittenstein commented Oct 21, 2020

Motivation

Adding [endpoint] proc-macro attribute. It should ease the implementation of apps' endpoints.

This macro implements:

  • Auto-parsing of the input calldata to Rust native types (see PR: CallData Decode API #181)

  • Will generate in a future PR the API schema in a JSON format (will be consumed by smapp). For now, it will gather the required data in compile-time.

@YaronWittenstein YaronWittenstein marked this pull request as ready for review Oct 29, 2020

#[derive(Debug, PartialEq, Copy, Clone, Hash)]
#[repr(transparent)]
pub struct Ptr(pub usize);
Copy link
Member

Choose a reason for hiding this comment

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

What's the usage of this? Was it supposed to be used in alloc func?

unsafe {
INIT.call_once(|| {
HOST = MaybeUninit::new(InnerHost::new());
});
Copy link
Member

@moshababo moshababo Nov 2, 2020

Choose a reason for hiding this comment

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

If INIT is ought to be initialized only once, why not using lazy_static! like previously?

Also, you previously applied a mutex lock - isn't it needed? Are you now relying on the client to protect against concurrent read/write?

#![allow(dead_code)]
#![allow(unreachable_code)]

extern crate alloc;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps calling it memory like previously is preferred - both so you could add additional functionality, and also to give better context to the alloc func.

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 prefer to keep the current name for now.
I don't expect it to change in the future.


use alloc::alloc::{alloc_zeroed, Layout};

pub fn alloc(n: usize) -> usize {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps specify what's the benefit of using alloc over something else.
Is it just because it provides zero-initialized bytes? Or also because of the allocator it is using?


f();

drop(guard)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't guard going to be dropped here anyway?

@@ -0,0 +1,271 @@
// use cfg_if::cfg_if;

Copy link
Member

Choose a reason for hiding this comment

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

Why commented?

@@ -173,7 +173,7 @@ fn storage() -> MutexGuard<'static, InnerStorage> {
mod tests {
use super::*;

use crate::memory::alloc;
use svm_sdk_alloc::alloc;
Copy link
Member

Choose a reason for hiding this comment

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

STORAGE instance init/access pattern is different than HOST - If there's no reason for that, it should be unified.

@YaronWittenstein YaronWittenstein merged commit 2d03e3c into master Nov 3, 2020
@YaronWittenstein YaronWittenstein deleted the endpoint-proc-macro branch Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants