Copilot commented on code in PR #682:
URL: https://github.com/apache/tvm-ffi/pull/682#discussion_r3634927865
##########
rust/tvm-ffi-macros/src/utils.rs:
##########
@@ -23,7 +23,9 @@ use std::env;
/// Get the tvm-rt crate name
/// \return The tvm-rt crate name
pub(crate) fn get_tvm_ffi_crate() -> TokenStream {
- if env::var("CARGO_PKG_NAME").unwrap() == "tvm-ffi" {
+ if env::var("CARGO_PKG_NAME").unwrap() == "tvm-ffi"
+ && env::var("CARGO_CRATE_NAME").unwrap() == "tvm_ffi"
Review Comment:
`get_tvm_ffi_crate()` uses `env::var(...).unwrap()` for both
`CARGO_PKG_NAME` and `CARGO_CRATE_NAME`, which can panic during proc-macro
expansion in non-Cargo contexts (e.g., some IDEs/tools) and yields an unhelpful
hard failure. It also still references "tvm-rt" in the doc comment, which
appears to be a stale name in this crate.
Consider using `unwrap_or_default()` (or `ok().as_deref()`) and treating
missing env vars as the external-crate case (emit `tvm_ffi`).
##########
rust/tvm-ffi/tests/test_match_any.rs:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use tvm_ffi::{match_any, Any, Array, Map, Shape, Tensor};
+
+#[test]
+fn matches_concrete_object_containers_in_source_order() {
+ fn classify(expr: Any) -> (&'static str, usize) {
+ match_any! {
+ expr {
+ Tensor(tensor)
+ if tensor.shape().len() == 2 => ("matrix",
tensor.shape().len()),
+ Tensor(tensor) => ("tensor", tensor.shape().len()),
+ Shape(shape) => ("shape", shape.len()),
+ Array::<i64>(array) => ("array", array.len()),
+ _ => ("unsupported", 0),
+ }
+ }
+ }
+
+ let matrix = Tensor::from_slice(&[0_f32; 6], &[2, 3]).unwrap();
+ let volume = Tensor::from_slice(&[0_f32; 24], &[2, 3, 4]).unwrap();
+ let shape = Shape::from([2_i64, 3, 4, 5]);
+ let array = [1_i64, 2, 3].into_iter().collect::<Array<i64>>();
+
+ assert_eq!(classify(Any::from(matrix)), ("matrix", 2));
+ assert_eq!(classify(Any::from(volume)), ("tensor", 3));
+ assert_eq!(classify(Any::from(shape)), ("shape", 4));
+ assert_eq!(classify(Any::from(array)), ("array", 3));
+ assert_eq!(
+ classify(Any::from(Map::<i64, i64>::default())),
+ ("unsupported", 0)
+ );
Review Comment:
The PR description promises (1) non-object values skip typed arms and go
straight to the `_` fallback, and (2) borrowed object handles can be passed via
an `AnyView` scrutinee. The new test only covers object-backed values; adding
assertions for a scalar (e.g. `i64`) and for an `AnyView` scrutinee would help
prevent regressions in these key semantics.
##########
rust/tvm-ffi-macros/src/match_any.rs:
##########
@@ -0,0 +1,161 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use proc_macro2::{Ident, Span, TokenStream};
+use quote::quote;
+use syn::parse::{Parse, ParseStream};
+use syn::{braced, parenthesized, Expr, Pat, Path, Result, Token};
+
+use crate::utils::get_tvm_ffi_crate;
+
+struct MatchAnyInput {
+ scrutinee: Expr,
+ arms: Vec<TypedArm>,
+ fallback: Expr,
+}
+
+struct TypedArm {
+ matcher: Path,
+ binding: Pat,
+ guard: Option<Expr>,
+ body: Expr,
+}
+
+impl Parse for MatchAnyInput {
+ fn parse(input: ParseStream<'_>) -> Result<Self> {
+ let scrutinee = input.call(Expr::parse_without_eager_brace)?;
+ let content;
+ braced!(content in input);
+
+ let mut arms = Vec::new();
+ let mut fallback = None;
+ while !content.is_empty() {
+ if fallback.is_some() {
+ return Err(content.error("the `_` fallback must be the final
arm"));
+ }
+
+ if content.peek(Token![_]) {
+ content.parse::<Token![_]>()?;
+ if content.peek(Token![if]) {
+ return Err(content.error("the `_` fallback cannot have a
guard"));
+ }
+ content.parse::<Token![=>]>()?;
+ fallback = Some(content.parse::<Expr>()?);
+ } else {
+ let matcher = content.parse::<Path>()?;
+ let binding_content;
+ parenthesized!(binding_content in content);
+ let binding = binding_content.parse::<Pat>()?;
+ if !binding_content.is_empty() {
+ return Err(binding_content.error("expected one binding
pattern"));
+ }
+ let guard = if content.peek(Token![if]) {
+ content.parse::<Token![if]>()?;
+ Some(content.parse::<Expr>()?)
+ } else {
+ None
+ };
+ content.parse::<Token![=>]>()?;
+ let body = content.parse::<Expr>()?;
+ arms.push(TypedArm {
+ matcher,
+ binding,
+ guard,
+ body,
+ });
+ }
+
+ if content.peek(Token![,]) {
+ content.parse::<Token![,]>()?;
+ } else if !content.is_empty() {
+ return Err(content.error("expected `,` between match_any!
arms"));
+ }
+ }
+
+ let fallback = fallback
+ .ok_or_else(|| content.error("match_any! requires a final `_`
fallback arm"))?;
+ Ok(Self {
+ scrutinee,
+ arms,
+ fallback,
+ })
+ }
+}
+
+pub fn expand(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
+ let input = syn::parse_macro_input!(input as MatchAnyInput);
+ expand_match_any(input).into()
+}
+
+fn expand_match_any(input: MatchAnyInput) -> TokenStream {
+ let tvm_ffi = get_tvm_ffi_crate();
+ let span = Span::mixed_site();
+ let source = Ident::new("__tvm_ffi_match_any_source", span);
+ let converted = Ident::new("__tvm_ffi_match_any_converted", span);
+ let view = Ident::new("__tvm_ffi_match_any_view", span);
+ let rejected = Ident::new("__tvm_ffi_match_any_rejected", span);
+ let scrutinee = input.scrutinee;
+ let fallback = input.fallback;
+ let dispatch_fallback = fallback.clone();
+ let arms = input.arms;
+ let dispatch = arms
+ .into_iter()
+ .rev()
+ .fold(quote!({ #dispatch_fallback }), |next, arm| {
+ let matcher = arm.matcher;
+ let binding = arm.binding;
+ let body = arm.body;
+ let matched = if let Some(guard) = arm.guard {
+ quote!(::core::result::Result::Ok(#binding) if #guard)
+ } else {
+ quote!(::core::result::Result::Ok(#binding))
+ };
+
+ quote! {
+ match ::core::convert::TryInto::<#matcher>::try_into(#view) {
+ #matched => { #body },
+ #rejected => {
+ ::core::mem::drop(#rejected);
+ #next
+ },
+ }
Review Comment:
`match_any!` currently uses `TryInto::<Matcher>::try_into(#view)` for each
arm. For the built-in TVM FFI types, those `TryFrom<AnyView>` impls often build
a formatted `Error` on mismatch (e.g. via `TryFromTemp` in
`rust/tvm-ffi/src/any.rs`), but `match_any!` discards the error and continues.
In hot dispatch code, this can introduce avoidable allocations/formatting work
for every rejected arm.
A more efficient approach would be to add a no-allocation cast/check API
(e.g. `AnyView::try_cast::<T>() -> Option<T>` or a dedicated pattern trait) and
have `match_any!` use that for arm testing, reserving full `Error` construction
for user-facing conversions.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
