[GitHub] [incubator-tvm] adelbertc commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods

[GitBox <gi...@apache.org>]

adelbertc commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r517787579



##########
File path: rust/tvm-macros/src/external.rs
##########
@@ -124,15 +151,21 @@ pub fn macro_impl(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
                         let ty: Type = *pat_type.ty.clone();
                         (ident, ty)
                     }
-                    _ => panic!(),
+                    _ => abort! { pat_type,

Review comment:
       Is `abort!` the standard way to blow up a macro?

##########
File path: rust/tvm-macros/src/object.rs
##########
@@ -75,6 +79,12 @@ pub fn macro_impl(input: proc_macro::TokenStream) -> TokenStream {
         _ => panic!("derive only works for structs"),
     };
 
+    let ref_derives = if derive {

Review comment:
       This is like if you mark something with a `no_derive` attribute you don't derive `Debug`? Naming is a bit wonky here

##########
File path: rust/tvm-rt/src/array.rs
##########
@@ -82,6 +82,13 @@ impl<T: IsObjectRef> Array<T> {
     }
 }
 
+impl<T: IsObjectRef> std::fmt::Debug for Array<T> {
+    fn fmt(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+        let as_vec: Vec<T> = self.clone().into_iter().collect();

Review comment:
       Instead of consuming `self` with `into_iter` could you have a method that returns an `Iterator` over references so you don't have to clone, since `write!` I assume doesn't need owned values? But this may not matter if `clone`ing these `Array`s isn't expected to be expensive.

##########
File path: rust/tvm-macros/src/external.rs
##########
@@ -17,12 +17,32 @@
  * under the License.
  */
 use proc_macro2::Span;
+use proc_macro_error::abort;
 use quote::quote;
 use syn::parse::{Parse, ParseStream, Result};
 
-use syn::{FnArg, Generics, Ident, Lit, Meta, NestedMeta, Pat, ReturnType, TraitItemMethod, Type};
+use syn::{FnArg, Signature, Attribute, token::Semi, Visibility, Generics, Ident, Lit, Meta, NestedMeta, Pat, ReturnType, Type};
+
+struct ExternalItem {

Review comment:
       What Greg said

##########
File path: rust/tvm-rt/src/object/object_ptr.rs
##########
@@ -147,14 +148,26 @@ impl Object {
     }
 }
 
+// impl fmt::Debug for Object {

Review comment:
       remove dead code




----------------------------------------------------------------
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