guan404ming commented on PR #399: URL: https://github.com/apache/tvm-ffi/pull/399#issuecomment-3819103124
Thanks for the feedback! After reviewing the design options and looking at real-world patterns, I agree that we should support implicit conversion. Here is some of my note to clarify all thing: ### Pros and Cons | Approach | Success Return | Error Return | Pros | Cons | |----------|---------------|--------------|------|------| | Factory functions (current) | `return ExpectedOk(x);` | `return ExpectedErr<T>(err);` | Explicit intent | Asymmetric API - Ok deduces type, Err requires explicit `<T>` | | Static methods only | `return Expected<T>::Ok(x);` | `return Expected<T>::Err(err);` | No extra functions | Verbose, type repeated | | Implicit conversion | `return x;` | `return Error(...);` | Concise, symmetric, follows C++ conventions | Slightly less explicit | ### C++ Standard Precedent From [cppreference std::expected](https://en.cppreference.com/w/cpp/utility/expected): >`std::expected<T,E>` provides implicit conversion from `T` for the success case, and uses `std::unexpected<E>` for errors. Similarly, `std::optional<T>` allows implicit conversion from `T`. ### XGrammar Current Pattern Looking at [XGrammar's exception.h](https://github.com/mlc-ai/xgrammar/blob/main/include/xgrammar/exception.h), they currently use traditional C++ exceptions: ```cpp // Current XGrammar (exceptions) Grammar Grammar::FromJSON(const std::string& json) { if (!valid) throw InvalidJSONError("..."); return grammar; } // With Expected + implicit conversion Expected<Grammar> Grammar::FromJSON(const std::string& json) { if (!valid) return Error("InvalidJSONError", "..."); return grammar; } ``` The implicit conversion approach makes migration from exception-based code more natural. ### Implementation I think to add implicit converting constructors would be ideal to handle this feat: ```cpp template <typename T> class Expected { public: Expected(T value) : data_(Any(std::move(value))) {} // implicit from T Expected(Error error) : data_(Any(std::move(error))) {} // implicit from Error // ... }; ``` This eliminates the asymmetry issue where `ExpectedOk` deduces type but `ExpectedErr<T>` requires explicit specification. In this way, we then could remove `ExpectedOk()`, `ExpectedErr()` factory functions and `Expected<T>::Ok()`, `Expected<T>::Err()` static methods accordingly. Does this approach sound reasonable? Happy to discuss if there are any concerns or alternative suggestions. -- 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]
