gemini-code-assist[bot] commented on code in PR #18883:
URL: https://github.com/apache/tvm/pull/18883#discussion_r2896408994
##########
tests/cpp/target_test.cc:
##########
@@ -222,6 +222,69 @@ TEST(TargetCreation, TargetParserProcessing) {
ASSERT_EQ(test_target->GetAttr<ffi::String>("mattr").value(), "cake");
}
+TEST(TargetCreation, RoundTripCanonicalizerFeatures) {
+ // Construct a target whose canonicalizer sets feature.test and transforms
mcpu
+ Target original(ffi::Map<ffi::String, ffi::Any>{
+ {"kind", ffi::String("TestTargetParser")},
+ {"mcpu", ffi::String("woof")},
+ });
+ ASSERT_EQ(original->GetAttr<ffi::String>("mcpu").value(), "super_woof");
+ ASSERT_EQ(original->GetAttr<bool>("feature.test").value(), true);
+
+ // Export to config and reconstruct
+ ffi::Map<ffi::String, ffi::Any> exported = original->ToConfig();
+ Target reconstructed(exported);
+
+ // Canonicalized attrs must survive the round-trip
+ // Note: mcpu gets canonicalized again (super_super_woof) because the
canonicalizer runs
+ ASSERT_TRUE(reconstructed->GetAttr<ffi::String>("mcpu").has_value());
+ ASSERT_EQ(reconstructed->GetAttr<bool>("feature.test").value(), true);
+ ASSERT_EQ(reconstructed->keys.size(), 1);
+ ASSERT_EQ(reconstructed->keys[0], "super");
+}
+
+TEST(TargetCreation, RoundTripCanonicalizerFeaturesNestedHost) {
+ // Construct a host target whose canonicalizer sets feature.test
+ Target host(ffi::Map<ffi::String, ffi::Any>{
+ {"kind", ffi::String("TestTargetParser")},
+ {"mcpu", ffi::String("woof")},
+ });
+ ASSERT_EQ(host->GetAttr<bool>("feature.test").value(), true);
+
+ // Attach it as host to another target
+ Target outer(ffi::Map<ffi::String, ffi::Any>{
+ {"kind", ffi::String("TestTargetKind")},
+ {"my_bool", true},
+ });
+ Target combined(outer, host);
+
+ // Export the outer target (includes nested host) and reconstruct
+ ffi::Map<ffi::String, ffi::Any> exported = combined->ToConfig();
+ Target reconstructed(exported);
+
+ // The nested host must reconstruct successfully with feature.* preserved
+ ffi::Optional<Target> reconstructed_host = reconstructed->GetHost();
+ ASSERT_TRUE(reconstructed_host.defined());
+ ASSERT_EQ(reconstructed_host.value()->GetAttr<bool>("feature.test").value(),
true);
+
ASSERT_TRUE(reconstructed_host.value()->GetAttr<ffi::String>("mcpu").has_value());
+}
+
+TEST(TargetCreationFail, UnknownNonFeatureKeyStillFails) {
+ // Verify that unknown non-feature.* keys still fail schema validation
+ ffi::Map<ffi::String, ffi::Any> config = {
+ {"kind", ffi::String("TestTargetParser")},
+ {"mcpu", ffi::String("woof")},
+ {"unknown_key", ffi::String("bad")},
+ };
+ bool failed = false;
+ try {
+ Target tgt(config);
+ } catch (...) {
+ failed = true;
+ }
+ ASSERT_EQ(failed, true);
Review Comment:
The `try/catch` block for testing exceptions can be simplified using gtest's
`ASSERT_THROW`. This is more idiomatic, makes the test's intent clearer, and is
consistent with other `EXPECT_THROW` usage in this file.
```suggestion
ASSERT_THROW({ Target(config); }, tvm::Error);
```
##########
src/target/target.cc:
##########
@@ -330,11 +330,38 @@ ObjectPtr<TargetNode>
TargetInternal::FromConfig(ffi::Map<ffi::String, ffi::Any>
target->host = std::nullopt;
}
- // Step 3: Use ConfigSchema to validate types, apply defaults, and run
canonicalizer
+ // Step 3: Extract any "feature.*" keys from the input config before schema
validation.
+ // These are canonicalizer-owned metadata that may appear in exported
configs (via ToConfig())
+ // but are not part of the target kind's declared schema. We preserve them
across round-trip
+ // and let the canonicalizer's output take priority if it re-emits the same
key.
+ std::unordered_map<std::string, ffi::Any> saved_features;
+ {
+ std::vector<ffi::String> feature_keys;
+ for (const auto& kv : config) {
+ std::string key_str(kv.first);
+ if (key_str.size() > 8 && key_str.substr(0, 8) == "feature.") {
Review Comment:
For improved performance and to avoid creating a temporary string, you can
use `std::string::compare` instead of `substr` for checking the prefix. This is
a small optimization but is generally good practice.
```suggestion
if (key_str.size() > 8 && key_str.compare(0, 8, "feature.") == 0) {
```
--
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]
