Rustify PackedFunc & Friends

[nhynes]

No description provided.

[nhynes]

cc @ehsanmok for review

Aren't PRs that have more red than green the best?

[ehsanmok]

Nice job! I really like the enum POD repr. Here're some small comments:

[ehsanmok]

Neat :)

[ehsanmok]

Can it be without clone after into_iter or something?

[nhynes]

that would require

- type PackedFunc = Fn(&[TVMArgValue]) -> TVMRetValue;
+ type PackedFunc = Fn(Vec<TVMArgValue>) -> TVMRetValue;

and the TVM module is surely not going to know how to Drop a Vec (nor can the frontend or runtime Drop a C++ std::vector).

I think that a better solution might be .as_tvm_value(), but that adds a lot of boilerplate code and it's not totally clear that rustc doesn't just optimize away the clone.

[nhynes]

Well, as_tvm_value seems correct. I guess I'll just make the macros more macro-y to eliminate the repetitive code.

[kazimuth]

You could eliminate the allocation on the rust side doing something like:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=8d4048c7235d7e7bf8074f1514a07350

This wouldn't be able to go through Builder though.

Also, mapping a slice of tuples to a pair of slices is actually surprisingly hard to do with macros. You could avoid doing it if you're able to just call into() twice for each macro, or else you can do some advanced macro magic to map over the slice.

[kazimuth]

This can be addressed later.

[ehsanmok]

@nhynes @tqchen LGTM

[kazimuth]

e: see my review

[tqchen]

@kazimuth can you also https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly

[kazimuth]

@tqchen sure

[nhynes]

I replaced into_tvm_value with to_tvm_value which takes &self. It copies the primitives and pointers, but this is actually correct since something needs to retain ownership of the strings and bytearrays.

[ehsanmok]

right! @nhynes if I remember correctly, into_tvm_value was necessary before esp. for box but it should be possible according to your changes now.

[nhynes]

It's no longer into because ffi::TVMValue doesn't own its contents. If, for instance, the frontend did into_tvm_value, there would be no guarantee that the pointed-to data wasn't dropped.

[ehsanmok]

@tqchen Could you merge this sooner please? I'm working on a feature which requires these fundamental changes and don't want to resolve too many conflicts.

[tqchen]

awaiting approval from @kazimuth

[kazimuth]

This can be addressed later.

[kazimuth]

Done @tqchen

[tqchen]

THanks, @nhynes , @kazimuth @ehsanmok , this is now merged