You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/09/02 16:46:45 UTC
[GitHub] [incubator-tvm] mwillsey opened a new pull request #6384: Add safe up/downcasting to the Rust object system
mwillsey opened a new pull request #6384:
URL: https://github.com/apache/incubator-tvm/pull/6384
I took a clean up pass over the TVM object system in the Rust bindings. There are still some things to improve, but I think this is worth landing on its own.
The upshot is now any `IsObjectRef` has a safe, typechecked `upcast` method to anything it inherits from. `downcast` is also typechecked, in that you may only attempt a downcast to from T to U if U is a subclass of T. There are some other minor cleanups in the Object derive-macro as well.
@jroesch
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch merged pull request #6384: Add safe up/downcasting to the Rust object system
Posted by GitBox <gi...@apache.org>.
jroesch merged pull request #6384:
URL: https://github.com/apache/incubator-tvm/pull/6384
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mwillsey commented on a change in pull request #6384: Add safe up/downcasting to the Rust object system
Posted by GitBox <gi...@apache.org>.
mwillsey commented on a change in pull request #6384:
URL: https://github.com/apache/incubator-tvm/pull/6384#discussion_r482543102
##########
File path: rust/tvm-rt/src/object/object_ptr.rs
##########
@@ -179,14 +172,6 @@ pub struct ObjectPtr<T: IsObject> {
pub ptr: NonNull<T>,
}
-fn inc_ref<T: IsObject>(ptr: NonNull<T>) {
- unsafe { ptr.as_ref().as_object().inc_ref() }
-}
-
-fn dec_ref<T: IsObject>(ptr: NonNull<T>) {
- unsafe { ptr.as_ref().as_object().dec_ref() }
-}
-
Review comment:
We still do something like this, but it's just in one place now, so I inlined it.
##########
File path: rust/tvm-macros/src/object.rs
##########
@@ -23,56 +23,67 @@ use quote::quote;
use syn::DeriveInput;
use syn::Ident;
-use crate::util::get_tvm_rt_crate;
+use crate::util::*;
pub fn macro_impl(input: proc_macro::TokenStream) -> TokenStream {
let tvm_rt_crate = get_tvm_rt_crate();
let result = quote! { #tvm_rt_crate::function::Result };
let error = quote! { #tvm_rt_crate::errors::Error };
let derive_input = syn::parse_macro_input!(input as DeriveInput);
- let payload_id = derive_input.ident;
-
- let mut type_key = None;
- let mut ref_name = None;
- let base = Some(Ident::new("base", Span::call_site()));
-
- for attr in derive_input.attrs {
- if attr.path.is_ident("type_key") {
- type_key = Some(attr.parse_meta().expect("foo"))
- }
-
- if attr.path.is_ident("ref_name") {
- ref_name = Some(attr.parse_meta().expect("foo"))
- }
- }
-
- let type_key = if let Some(syn::Meta::NameValue(name_value)) = type_key {
- match name_value.lit {
- syn::Lit::Str(type_key) => type_key,
- _ => panic!("foo"),
- }
- } else {
- panic!("bar");
- };
-
- let ref_name = if let Some(syn::Meta::NameValue(name_value)) = ref_name {
- match name_value.lit {
- syn::Lit::Str(ref_name) => ref_name,
- _ => panic!("foo"),
- }
- } else {
- panic!("bar");
+ let payload_id = derive_input.ident.clone();
+
+ let type_key = get_attr(&derive_input, "type_key")
+ .map(attr_to_str)
+ .expect("Failed to get type_key");
+
+ let ref_id = get_attr(&derive_input, "ref_name")
+ .map(|a| Ident::new(attr_to_str(a).value().as_str(), Span::call_site()))
+ .unwrap_or_else(|| {
+ let id = payload_id.to_string();
+ let suffixes = ["Node", "Obj"];
+ if let Some(suf) = suffixes
+ .iter()
+ .find(|&suf| id.len() > suf.len() && id.ends_with(suf))
+ {
+ Ident::new(&id[..suf.len() - 4], payload_id.span())
Review comment:
good catch!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] robo-corg commented on a change in pull request #6384: Add safe up/downcasting to the Rust object system
Posted by GitBox <gi...@apache.org>.
robo-corg commented on a change in pull request #6384:
URL: https://github.com/apache/incubator-tvm/pull/6384#discussion_r482304451
##########
File path: rust/tvm-macros/src/object.rs
##########
@@ -23,56 +23,67 @@ use quote::quote;
use syn::DeriveInput;
use syn::Ident;
-use crate::util::get_tvm_rt_crate;
+use crate::util::*;
pub fn macro_impl(input: proc_macro::TokenStream) -> TokenStream {
let tvm_rt_crate = get_tvm_rt_crate();
let result = quote! { #tvm_rt_crate::function::Result };
let error = quote! { #tvm_rt_crate::errors::Error };
let derive_input = syn::parse_macro_input!(input as DeriveInput);
- let payload_id = derive_input.ident;
-
- let mut type_key = None;
- let mut ref_name = None;
- let base = Some(Ident::new("base", Span::call_site()));
-
- for attr in derive_input.attrs {
- if attr.path.is_ident("type_key") {
- type_key = Some(attr.parse_meta().expect("foo"))
- }
-
- if attr.path.is_ident("ref_name") {
- ref_name = Some(attr.parse_meta().expect("foo"))
- }
- }
-
- let type_key = if let Some(syn::Meta::NameValue(name_value)) = type_key {
- match name_value.lit {
- syn::Lit::Str(type_key) => type_key,
- _ => panic!("foo"),
- }
- } else {
- panic!("bar");
- };
-
- let ref_name = if let Some(syn::Meta::NameValue(name_value)) = ref_name {
- match name_value.lit {
- syn::Lit::Str(ref_name) => ref_name,
- _ => panic!("foo"),
- }
- } else {
- panic!("bar");
+ let payload_id = derive_input.ident.clone();
+
+ let type_key = get_attr(&derive_input, "type_key")
+ .map(attr_to_str)
+ .expect("Failed to get type_key");
+
+ let ref_id = get_attr(&derive_input, "ref_name")
+ .map(|a| Ident::new(attr_to_str(a).value().as_str(), Span::call_site()))
+ .unwrap_or_else(|| {
+ let id = payload_id.to_string();
+ let suffixes = ["Node", "Obj"];
+ if let Some(suf) = suffixes
+ .iter()
+ .find(|&suf| id.len() > suf.len() && id.ends_with(suf))
+ {
+ Ident::new(&id[..suf.len() - 4], payload_id.span())
Review comment:
What does this do? It seems like `suf.len() - 4` will only ever be 0 or -1. Should this be something like `id.len() - suf.len()`?
##########
File path: rust/tvm-rt/src/object/object_ptr.rs
##########
@@ -179,14 +172,6 @@ pub struct ObjectPtr<T: IsObject> {
pub ptr: NonNull<T>,
}
-fn inc_ref<T: IsObject>(ptr: NonNull<T>) {
- unsafe { ptr.as_ref().as_object().inc_ref() }
-}
-
-fn dec_ref<T: IsObject>(ptr: NonNull<T>) {
- unsafe { ptr.as_ref().as_object().dec_ref() }
-}
-
Review comment:
👍 I never noticed this but presumably you are removing this because this is not actually safe?
##########
File path: rust/tvm-rt/src/object/object_ptr.rs
##########
@@ -257,9 +250,7 @@ impl<T: IsObject> ObjectPtr<T> {
if is_derived {
// NB: self gets dropped here causng a dec ref which we need to migtigate with an inc ref before it is dropped.
Review comment:
I think this comment might be out of date now?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org