From 211bec2871da597f9f3fd81df7faffea1754437e Mon Sep 17 00:00:00 2001 From: Jeremy Meyer Date: Tue, 4 Jun 2024 14:22:03 -0700 Subject: First pass at flagged resources This gets the main parts of resource flagging in place and the basic use case of flagging with an xml attribute working. Test: Automated Bug: 329436914 Flag: EXEMPT Aconfig not supported on host tools Change-Id: Id2b5ba450d05da00a922e98ca204b6e5aa6c6c24 --- tools/aapt2/ResourceTable.cpp | 55 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) (limited to 'tools/aapt2/ResourceTable.cpp') diff --git a/tools/aapt2/ResourceTable.cpp b/tools/aapt2/ResourceTable.cpp index a3b0b45df5c3..1cdb71551d5d 100644 --- a/tools/aapt2/ResourceTable.cpp +++ b/tools/aapt2/ResourceTable.cpp @@ -231,6 +231,47 @@ bool ResourceEntry::HasDefaultValue() const { return false; } +ResourceTable::CollisionResult ResourceTable::ResolveFlagCollision(FlagStatus existing, + FlagStatus incoming) { + switch (existing) { + case FlagStatus::NoFlag: + switch (incoming) { + case FlagStatus::NoFlag: + return CollisionResult::kConflict; + case FlagStatus::Disabled: + return CollisionResult::kKeepOriginal; + case FlagStatus::Enabled: + return CollisionResult::kTakeNew; + default: + return CollisionResult::kConflict; + } + case FlagStatus::Disabled: + switch (incoming) { + case FlagStatus::NoFlag: + return CollisionResult::kTakeNew; + case FlagStatus::Disabled: + return CollisionResult::kKeepOriginal; + case FlagStatus::Enabled: + return CollisionResult::kTakeNew; + default: + return CollisionResult::kConflict; + } + case FlagStatus::Enabled: + switch (incoming) { + case FlagStatus::NoFlag: + return CollisionResult::kKeepOriginal; + case FlagStatus::Disabled: + return CollisionResult::kKeepOriginal; + case FlagStatus::Enabled: + return CollisionResult::kConflict; + default: + return CollisionResult::kConflict; + } + default: + return CollisionResult::kConflict; + } +} + // The default handler for collisions. // // Typically, a weak value will be overridden by a strong value. An existing weak @@ -564,16 +605,21 @@ bool ResourceTable::AddResource(NewResource&& res, android::IDiagnostics* diag) if (!config_value->value) { // Resource does not exist, add it now. config_value->value = std::move(res.value); + config_value->flag_status = res.flag_status; } else { // When validation is enabled, ensure that a resource cannot have multiple values defined for - // the same configuration. - auto result = validate ? ResolveValueCollision(config_value->value.get(), res.value.get()) + // the same configuration unless protected by flags. + auto result = validate ? ResolveFlagCollision(config_value->flag_status, res.flag_status) : CollisionResult::kKeepBoth; + if (result == CollisionResult::kConflict) { + result = ResolveValueCollision(config_value->value.get(), res.value.get()); + } switch (result) { case CollisionResult::kKeepBoth: // Insert the value ignoring for duplicate configurations entry->values.push_back(util::make_unique(res.config, res.product)); entry->values.back()->value = std::move(res.value); + entry->values.back()->flag_status = res.flag_status; break; case CollisionResult::kTakeNew: @@ -735,6 +781,11 @@ NewResourceBuilder& NewResourceBuilder::SetAllowMangled(bool allow_mangled) { return *this; } +NewResourceBuilder& NewResourceBuilder::SetFlagStatus(FlagStatus flag_status) { + res_.flag_status = flag_status; + return *this; +} + NewResource NewResourceBuilder::Build() { return std::move(res_); } -- cgit v1.2.3-59-g8ed1b From d52bd6858a6b2913f46e57bca0d96d5212416f10 Mon Sep 17 00:00:00 2001 From: Jeremy Meyer Date: Wed, 14 Aug 2024 11:16:58 -0700 Subject: Flag support for bag resource types Test: Automated Bug: 329436914 Flag: EXEMPT Aconfig not supported on host tools Change-Id: I891c93c3ffcab172d28701b44a80c50f1e24d99e --- .../ResourceFlaggingTest.java | 25 +++++++-- tools/aapt2/ResourceParser.cpp | 61 ++++++++++++++-------- tools/aapt2/ResourceParser.h | 2 + tools/aapt2/ResourceTable.cpp | 7 ++- tools/aapt2/ResourceTable.h | 2 - tools/aapt2/ResourceValues.cpp | 11 ++++ tools/aapt2/ResourceValues.h | 14 +++++ tools/aapt2/Resources.proto | 5 +- tools/aapt2/cmd/Link.cpp | 2 +- tools/aapt2/format/proto/ProtoDeserialize.cpp | 26 ++++++--- tools/aapt2/format/proto/ProtoSerialize.cpp | 5 +- .../FlaggedResourcesTest/Android.bp | 2 + .../FlaggedResourcesTest/res/values/ints.xml | 9 ++++ .../FlaggedResourcesTest/res/values/strings.xml | 7 +++ tools/aapt2/link/FlagDisabledResourceRemover.cpp | 7 ++- tools/aapt2/link/TableMerger.cpp | 5 +- 16 files changed, 144 insertions(+), 46 deletions(-) create mode 100644 tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/ints.xml (limited to 'tools/aapt2/ResourceTable.cpp') diff --git a/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java b/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java index c1e357864fff..471b4021545a 100644 --- a/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java +++ b/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java @@ -68,6 +68,16 @@ public class ResourceFlaggingTest { assertThat(getBoolean("res3")).isTrue(); } + @Test + public void testFlagDisabledStringArrayElement() { + assertThat(getStringArray("strarr1")).isEqualTo(new String[]{"one", "two", "three"}); + } + + @Test + public void testFlagDisabledIntArrayElement() { + assertThat(getIntArray("intarr1")).isEqualTo(new int[]{1, 2, 3}); + } + private boolean getBoolean(String name) { int resId = mResources.getIdentifier( name, @@ -77,13 +87,22 @@ public class ResourceFlaggingTest { return mResources.getBoolean(resId); } - private String getString(String name) { + private String[] getStringArray(String name) { + int resId = mResources.getIdentifier( + name, + "array", + "com.android.intenal.flaggedresources"); + assertThat(resId).isNotEqualTo(0); + return mResources.getStringArray(resId); + } + + private int[] getIntArray(String name) { int resId = mResources.getIdentifier( name, - "string", + "array", "com.android.intenal.flaggedresources"); assertThat(resId).isNotEqualTo(0); - return mResources.getString(resId); + return mResources.getIntArray(resId); } private String extractApkAndGetPath(int id) throws Exception { diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp index 1c85e9ff231b..a5aecc855707 100644 --- a/tools/aapt2/ResourceParser.cpp +++ b/tools/aapt2/ResourceParser.cpp @@ -151,6 +151,7 @@ static bool AddResourcesToTable(ResourceTable* table, android::IDiagnostics* dia } if (res->value != nullptr) { + res->value->SetFlagStatus(res->flag_status); // Attach the comment, source and config to the value. res->value->SetComment(std::move(res->comment)); res->value->SetSource(std::move(res->source)); @@ -546,30 +547,11 @@ bool ResourceParser::ParseResource(xml::XmlPullParser* parser, }); std::string resource_type = parser->element_name(); - std::optional flag = - xml::FindAttribute(parser, "http://schemas.android.com/apk/res/android", "featureFlag"); - out_resource->flag_status = FlagStatus::NoFlag; - if (flag) { - auto flag_it = options_.feature_flag_values.find(flag.value()); - if (flag_it == options_.feature_flag_values.end()) { - diag_->Error(android::DiagMessage(source_.WithLine(parser->line_number())) - << "Resource flag value undefined"); - return false; - } - const auto& flag_properties = flag_it->second; - if (!flag_properties.read_only) { - diag_->Error(android::DiagMessage(source_.WithLine(parser->line_number())) - << "Only read only flags may be used with resources"); - return false; - } - if (!flag_properties.enabled.has_value()) { - diag_->Error(android::DiagMessage(source_.WithLine(parser->line_number())) - << "Only flags with a value may be used with resources"); - return false; - } - out_resource->flag_status = - flag_properties.enabled.value() ? FlagStatus::Enabled : FlagStatus::Disabled; + auto flag_status = GetFlagStatus(parser); + if (!flag_status) { + return false; } + out_resource->flag_status = flag_status.value(); // The value format accepted for this resource. uint32_t resource_format = 0u; @@ -751,6 +733,33 @@ bool ResourceParser::ParseResource(xml::XmlPullParser* parser, return false; } +std::optional ResourceParser::GetFlagStatus(xml::XmlPullParser* parser) { + auto flag_status = FlagStatus::NoFlag; + + std::optional flag = xml::FindAttribute(parser, xml::kSchemaAndroid, "featureFlag"); + if (flag) { + auto flag_it = options_.feature_flag_values.find(flag.value()); + if (flag_it == options_.feature_flag_values.end()) { + diag_->Error(android::DiagMessage(source_.WithLine(parser->line_number())) + << "Resource flag value undefined"); + return {}; + } + const auto& flag_properties = flag_it->second; + if (!flag_properties.read_only) { + diag_->Error(android::DiagMessage(source_.WithLine(parser->line_number())) + << "Only read only flags may be used with resources"); + return {}; + } + if (!flag_properties.enabled.has_value()) { + diag_->Error(android::DiagMessage(source_.WithLine(parser->line_number())) + << "Only flags with a value may be used with resources"); + return {}; + } + flag_status = flag_properties.enabled.value() ? FlagStatus::Enabled : FlagStatus::Disabled; + } + return flag_status; +} + bool ResourceParser::ParseItem(xml::XmlPullParser* parser, ParsedResource* out_resource, const uint32_t format) { @@ -1657,12 +1666,18 @@ bool ResourceParser::ParseArrayImpl(xml::XmlPullParser* parser, const std::string& element_namespace = parser->element_namespace(); const std::string& element_name = parser->element_name(); if (element_namespace.empty() && element_name == "item") { + auto flag_status = GetFlagStatus(parser); + if (!flag_status) { + error = true; + continue; + } std::unique_ptr item = ParseXml(parser, typeMask, kNoRawString); if (!item) { diag_->Error(android::DiagMessage(item_source) << "could not parse array item"); error = true; continue; } + item->SetFlagStatus(flag_status.value()); item->SetSource(item_source); array->elements.emplace_back(std::move(item)); diff --git a/tools/aapt2/ResourceParser.h b/tools/aapt2/ResourceParser.h index 45d41c193cb4..442dea89ef40 100644 --- a/tools/aapt2/ResourceParser.h +++ b/tools/aapt2/ResourceParser.h @@ -85,6 +85,8 @@ class ResourceParser { private: DISALLOW_COPY_AND_ASSIGN(ResourceParser); + std::optional GetFlagStatus(xml::XmlPullParser* parser); + std::optional CreateFlattenSubTree(xml::XmlPullParser* parser); // Parses the XML subtree as a StyleString (flattened XML representation for strings with diff --git a/tools/aapt2/ResourceTable.cpp b/tools/aapt2/ResourceTable.cpp index 1cdb71551d5d..7a4f40e471d2 100644 --- a/tools/aapt2/ResourceTable.cpp +++ b/tools/aapt2/ResourceTable.cpp @@ -605,12 +605,12 @@ bool ResourceTable::AddResource(NewResource&& res, android::IDiagnostics* diag) if (!config_value->value) { // Resource does not exist, add it now. config_value->value = std::move(res.value); - config_value->flag_status = res.flag_status; } else { // When validation is enabled, ensure that a resource cannot have multiple values defined for // the same configuration unless protected by flags. - auto result = validate ? ResolveFlagCollision(config_value->flag_status, res.flag_status) - : CollisionResult::kKeepBoth; + auto result = + validate ? ResolveFlagCollision(config_value->value->GetFlagStatus(), res.flag_status) + : CollisionResult::kKeepBoth; if (result == CollisionResult::kConflict) { result = ResolveValueCollision(config_value->value.get(), res.value.get()); } @@ -619,7 +619,6 @@ bool ResourceTable::AddResource(NewResource&& res, android::IDiagnostics* diag) // Insert the value ignoring for duplicate configurations entry->values.push_back(util::make_unique(res.config, res.product)); entry->values.back()->value = std::move(res.value); - entry->values.back()->flag_status = res.flag_status; break; case CollisionResult::kTakeNew: diff --git a/tools/aapt2/ResourceTable.h b/tools/aapt2/ResourceTable.h index 4f76e7d3a2b7..cba6b70cfbd6 100644 --- a/tools/aapt2/ResourceTable.h +++ b/tools/aapt2/ResourceTable.h @@ -104,8 +104,6 @@ class ResourceConfigValue { // The actual Value. std::unique_ptr value; - FlagStatus flag_status = FlagStatus::NoFlag; - ResourceConfigValue(const android::ConfigDescription& config, android::StringPiece product) : config(config), product(product) { } diff --git a/tools/aapt2/ResourceValues.cpp b/tools/aapt2/ResourceValues.cpp index 166b01bd9154..b75e87c90128 100644 --- a/tools/aapt2/ResourceValues.cpp +++ b/tools/aapt2/ResourceValues.cpp @@ -971,6 +971,16 @@ void Array::Print(std::ostream* out) const { *out << "(array) [" << util::Joiner(elements, ", ") << "]"; } +void Array::RemoveFlagDisabledElements() { + const auto end_iter = elements.end(); + const auto remove_iter = std::stable_partition( + elements.begin(), end_iter, [](const std::unique_ptr& item) -> bool { + return item->GetFlagStatus() != FlagStatus::Disabled; + }); + + elements.erase(remove_iter, end_iter); +} + bool Plural::Equals(const Value* value) const { const Plural* other = ValueCast(value); if (!other) { @@ -1092,6 +1102,7 @@ template std::unique_ptr CopyValueFields(std::unique_ptr new_value, const T* value) { new_value->SetSource(value->GetSource()); new_value->SetComment(value->GetComment()); + new_value->SetFlagStatus(value->GetFlagStatus()); return new_value; } diff --git a/tools/aapt2/ResourceValues.h b/tools/aapt2/ResourceValues.h index 5192c2be1f98..a1b1839b19ef 100644 --- a/tools/aapt2/ResourceValues.h +++ b/tools/aapt2/ResourceValues.h @@ -65,6 +65,14 @@ class Value { return translatable_; } + void SetFlagStatus(FlagStatus val) { + flag_status_ = val; + } + + FlagStatus GetFlagStatus() const { + return flag_status_; + } + // Returns the source where this value was defined. const android::Source& GetSource() const { return source_; @@ -109,6 +117,10 @@ class Value { // of brevity and readability. Default implementation just calls Print(). virtual void PrettyPrint(text::Printer* printer) const; + // Removes any part of the value that is beind a disabled flag. + virtual void RemoveFlagDisabledElements() { + } + friend std::ostream& operator<<(std::ostream& out, const Value& value); protected: @@ -116,6 +128,7 @@ class Value { std::string comment_; bool weak_ = false; bool translatable_ = true; + FlagStatus flag_status_ = FlagStatus::NoFlag; private: virtual Value* TransformValueImpl(ValueTransformer& transformer) const = 0; @@ -346,6 +359,7 @@ struct Array : public TransformableValue> { bool Equals(const Value* value) const override; void Print(std::ostream* out) const override; + void RemoveFlagDisabledElements() override; }; struct Plural : public TransformableValue> { diff --git a/tools/aapt2/Resources.proto b/tools/aapt2/Resources.proto index 2ecc82ae4792..5c6408940b34 100644 --- a/tools/aapt2/Resources.proto +++ b/tools/aapt2/Resources.proto @@ -246,7 +246,7 @@ message Entry { message ConfigValue { Configuration config = 1; Value value = 2; - uint32 flag_status = 3; + reserved 3; } // The generic meta-data for every value in a resource table. @@ -280,6 +280,9 @@ message Item { Id id = 6; Primitive prim = 7; } + + // The status of the flag the value is behind if any + uint32 flag_status = 8; } // A CompoundValue is an abstract type. It represents a value that is a made of other values. diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp index 56f52885b36d..be63f82b30cf 100644 --- a/tools/aapt2/cmd/Link.cpp +++ b/tools/aapt2/cmd/Link.cpp @@ -1878,7 +1878,7 @@ class Linker { for (auto& type : package->types) { for (auto& entry : type->entries) { for (auto& config_value : entry->values) { - if (config_value->flag_status == FlagStatus::Disabled) { + if (config_value->value->GetFlagStatus() == FlagStatus::Disabled) { config_value->value->Accept(&visitor); } } diff --git a/tools/aapt2/format/proto/ProtoDeserialize.cpp b/tools/aapt2/format/proto/ProtoDeserialize.cpp index aaab3158f61e..55f5e5668a16 100644 --- a/tools/aapt2/format/proto/ProtoDeserialize.cpp +++ b/tools/aapt2/format/proto/ProtoDeserialize.cpp @@ -534,8 +534,6 @@ static bool DeserializePackageFromPb(const pb::Package& pb_package, const ResStr return false; } - config_value->flag_status = (FlagStatus)pb_config_value.flag_status(); - config_value->value = DeserializeValueFromPb(pb_config_value.value(), src_pool, config, &out_table->string_pool, files, out_error); if (config_value->value == nullptr) { @@ -877,11 +875,12 @@ std::unique_ptr DeserializeValueFromPb(const pb::Value& pb_value, return value; } -std::unique_ptr DeserializeItemFromPb(const pb::Item& pb_item, - const android::ResStringPool& src_pool, - const ConfigDescription& config, - android::StringPool* value_pool, - io::IFileCollection* files, std::string* out_error) { +std::unique_ptr DeserializeItemFromPbInternal(const pb::Item& pb_item, + const android::ResStringPool& src_pool, + const ConfigDescription& config, + android::StringPool* value_pool, + io::IFileCollection* files, + std::string* out_error) { switch (pb_item.value_case()) { case pb::Item::kRef: { const pb::Reference& pb_ref = pb_item.ref(); @@ -1010,6 +1009,19 @@ std::unique_ptr DeserializeItemFromPb(const pb::Item& pb_item, return {}; } +std::unique_ptr DeserializeItemFromPb(const pb::Item& pb_item, + const android::ResStringPool& src_pool, + const ConfigDescription& config, + android::StringPool* value_pool, + io::IFileCollection* files, std::string* out_error) { + auto item = + DeserializeItemFromPbInternal(pb_item, src_pool, config, value_pool, files, out_error); + if (item) { + item->SetFlagStatus((FlagStatus)pb_item.flag_status()); + } + return item; +} + std::unique_ptr DeserializeXmlResourceFromPb(const pb::XmlNode& pb_node, std::string* out_error) { if (!pb_node.has_element()) { diff --git a/tools/aapt2/format/proto/ProtoSerialize.cpp b/tools/aapt2/format/proto/ProtoSerialize.cpp index c1e15bcf9f70..5772b3b0b3e6 100644 --- a/tools/aapt2/format/proto/ProtoSerialize.cpp +++ b/tools/aapt2/format/proto/ProtoSerialize.cpp @@ -426,7 +426,6 @@ void SerializeTableToPb(const ResourceTable& table, pb::ResourceTable* out_table pb_config_value->mutable_config()->set_product(config_value->product); SerializeValueToPb(*config_value->value, pb_config_value->mutable_value(), source_pool.get()); - pb_config_value->set_flag_status((uint32_t)config_value->flag_status); } } } @@ -720,6 +719,9 @@ void SerializeValueToPb(const Value& value, pb::Value* out_value, android::Strin if (src_pool != nullptr) { SerializeSourceToPb(value.GetSource(), src_pool, out_value->mutable_source()); } + if (out_value->has_item()) { + out_value->mutable_item()->set_flag_status((uint32_t)value.GetFlagStatus()); + } } void SerializeItemToPb(const Item& item, pb::Item* out_item) { @@ -727,6 +729,7 @@ void SerializeItemToPb(const Item& item, pb::Item* out_item) { ValueSerializer serializer(&value, nullptr); item.Accept(&serializer); out_item->MergeFrom(value.item()); + out_item->set_flag_status((uint32_t)item.GetFlagStatus()); } void SerializeCompiledFileToPb(const ResourceFile& file, pb::internal::CompiledFile* out_file) { diff --git a/tools/aapt2/integration-tests/FlaggedResourcesTest/Android.bp b/tools/aapt2/integration-tests/FlaggedResourcesTest/Android.bp index 5932271d4d28..4866d2c83c71 100644 --- a/tools/aapt2/integration-tests/FlaggedResourcesTest/Android.bp +++ b/tools/aapt2/integration-tests/FlaggedResourcesTest/Android.bp @@ -28,11 +28,13 @@ genrule { srcs: [ "res/values/bools.xml", "res/values/bools2.xml", + "res/values/ints.xml", "res/values/strings.xml", ], out: [ "values_bools.arsc.flat", "values_bools2.arsc.flat", + "values_ints.arsc.flat", "values_strings.arsc.flat", ], cmd: "$(location aapt2) compile $(in) -o $(genDir) " + diff --git a/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/ints.xml b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/ints.xml new file mode 100644 index 000000000000..26a5c40bb338 --- /dev/null +++ b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/ints.xml @@ -0,0 +1,9 @@ + + + + 1 + 2 + 666 + 3 + + \ No newline at end of file diff --git a/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/strings.xml b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/strings.xml index 5c0fca16fe39..3cbb928a64cc 100644 --- a/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/strings.xml +++ b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/strings.xml @@ -3,4 +3,11 @@ plain string DONTFIND + + + one + two + remove + three + \ No newline at end of file diff --git a/tools/aapt2/link/FlagDisabledResourceRemover.cpp b/tools/aapt2/link/FlagDisabledResourceRemover.cpp index e3289e2a173a..3ac17625023e 100644 --- a/tools/aapt2/link/FlagDisabledResourceRemover.cpp +++ b/tools/aapt2/link/FlagDisabledResourceRemover.cpp @@ -32,12 +32,17 @@ static bool KeepResourceEntry(const std::unique_ptr& entry) { const auto remove_iter = std::stable_partition(entry->values.begin(), end_iter, [](const std::unique_ptr& value) -> bool { - return value->flag_status != FlagStatus::Disabled; + return value->value->GetFlagStatus() != FlagStatus::Disabled; }); bool keep = remove_iter != entry->values.begin(); entry->values.erase(remove_iter, end_iter); + + for (auto& value : entry->values) { + value->value->RemoveFlagDisabledElements(); + } + return keep; } diff --git a/tools/aapt2/link/TableMerger.cpp b/tools/aapt2/link/TableMerger.cpp index 1942fc11c32e..37a039e9528f 100644 --- a/tools/aapt2/link/TableMerger.cpp +++ b/tools/aapt2/link/TableMerger.cpp @@ -212,8 +212,8 @@ static ResourceTable::CollisionResult MergeConfigValue( collision_result = ResolveMergeCollision(override_styles_instead_of_overlaying, dst_value, src_value, pool); } else { - collision_result = ResourceTable::ResolveFlagCollision(dst_config_value->flag_status, - src_config_value->flag_status); + collision_result = + ResourceTable::ResolveFlagCollision(dst_value->GetFlagStatus(), src_value->GetFlagStatus()); if (collision_result == CollisionResult::kConflict) { collision_result = ResourceTable::ResolveValueCollision(dst_value, src_value); } @@ -295,7 +295,6 @@ bool TableMerger::DoMerge(const android::Source& src, ResourceTablePackage* src_ } else { dst_config_value = dst_entry->FindOrCreateValue(src_config_value->config, src_config_value->product); - dst_config_value->flag_status = src_config_value->flag_status; } // Continue if we're taking the new resource. -- cgit v1.2.3-59-g8ed1b From a62e6bece14f1e2cdcaf21215cdefbf7e7243777 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Tue, 27 Aug 2024 14:20:51 -0700 Subject: [aapt2] Fix the sorting in ResourceTable for flagged entries + a few C++ style fixes that are supposed to be noops, but with the existing perf test regression record I'm not sure anymore Flag: EXEMPT bugfix Test: build + boot + aapt2_tests Change-Id: I918448ddc3840e1a180466d794eb81e8ec4b9640 --- tools/aapt2/ResourceTable.cpp | 83 +++++++++++++++++++------------------------ 1 file changed, 37 insertions(+), 46 deletions(-) (limited to 'tools/aapt2/ResourceTable.cpp') diff --git a/tools/aapt2/ResourceTable.cpp b/tools/aapt2/ResourceTable.cpp index 7a4f40e471d2..97514599c0b1 100644 --- a/tools/aapt2/ResourceTable.cpp +++ b/tools/aapt2/ResourceTable.cpp @@ -50,21 +50,21 @@ bool less_than_type(const std::unique_ptr& lhs, template bool less_than_struct_with_name(const std::unique_ptr& lhs, StringPiece rhs) { - return lhs->name.compare(0, lhs->name.size(), rhs.data(), rhs.size()) < 0; + return lhs->name < rhs; } template bool greater_than_struct_with_name(StringPiece lhs, const std::unique_ptr& rhs) { - return rhs->name.compare(0, rhs->name.size(), lhs.data(), lhs.size()) > 0; + return rhs->name > lhs; } template struct NameEqualRange { bool operator()(const std::unique_ptr& lhs, StringPiece rhs) const { - return less_than_struct_with_name(lhs, rhs); + return less_than_struct_with_name(lhs, rhs); } bool operator()(StringPiece lhs, const std::unique_ptr& rhs) const { - return greater_than_struct_with_name(lhs, rhs); + return greater_than_struct_with_name(lhs, rhs); } }; @@ -74,7 +74,7 @@ bool less_than_struct_with_name_and_id(const T& lhs, if (lhs.id != rhs.second) { return lhs.id < rhs.second; } - return lhs.name.compare(0, lhs.name.size(), rhs.first.data(), rhs.first.size()) < 0; + return lhs.name < rhs.first; } template @@ -90,14 +90,16 @@ struct ConfigKey { StringPiece product; }; -template -bool lt_config_key_ref(const T& lhs, const ConfigKey& rhs) { - int cmp = lhs->config.compare(*rhs.config); - if (cmp == 0) { - cmp = StringPiece(lhs->product).compare(rhs.product); +struct lt_config_key_ref { + template + bool operator()(const T& lhs, const ConfigKey& rhs) const noexcept { + int cmp = lhs->config.compare(*rhs.config); + if (cmp == 0) { + cmp = lhs->product.compare(rhs.product); + } + return cmp < 0; } - return cmp < 0; -} +}; } // namespace @@ -159,10 +161,10 @@ ResourceEntry* ResourceTableType::FindOrCreateEntry(android::StringPiece name) { ResourceConfigValue* ResourceEntry::FindValue(const ConfigDescription& config, android::StringPiece product) { auto iter = std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product}, - lt_config_key_ref>); + lt_config_key_ref()); if (iter != values.end()) { ResourceConfigValue* value = iter->get(); - if (value->config == config && StringPiece(value->product) == product) { + if (value->config == config && value->product == product) { return value; } } @@ -172,10 +174,10 @@ ResourceConfigValue* ResourceEntry::FindValue(const ConfigDescription& config, const ResourceConfigValue* ResourceEntry::FindValue(const android::ConfigDescription& config, android::StringPiece product) const { auto iter = std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product}, - lt_config_key_ref>); + lt_config_key_ref()); if (iter != values.end()) { ResourceConfigValue* value = iter->get(); - if (value->config == config && StringPiece(value->product) == product) { + if (value->config == config && value->product == product) { return value; } } @@ -185,10 +187,10 @@ const ResourceConfigValue* ResourceEntry::FindValue(const android::ConfigDescrip ResourceConfigValue* ResourceEntry::FindOrCreateValue(const ConfigDescription& config, StringPiece product) { auto iter = std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product}, - lt_config_key_ref>); + lt_config_key_ref()); if (iter != values.end()) { ResourceConfigValue* value = iter->get(); - if (value->config == config && StringPiece(value->product) == product) { + if (value->config == config && value->product == product) { return value; } } @@ -199,36 +201,21 @@ ResourceConfigValue* ResourceEntry::FindOrCreateValue(const ConfigDescription& c std::vector ResourceEntry::FindAllValues(const ConfigDescription& config) { std::vector results; - - auto iter = values.begin(); + auto iter = + std::lower_bound(values.begin(), values.end(), ConfigKey{&config, ""}, lt_config_key_ref()); for (; iter != values.end(); ++iter) { ResourceConfigValue* value = iter->get(); - if (value->config == config) { - results.push_back(value); - ++iter; + if (value->config != config) { break; } - } - - for (; iter != values.end(); ++iter) { - ResourceConfigValue* value = iter->get(); - if (value->config == config) { - results.push_back(value); - } + results.push_back(value); } return results; } bool ResourceEntry::HasDefaultValue() const { - const ConfigDescription& default_config = ConfigDescription::DefaultConfig(); - // The default config should be at the top of the list, since the list is sorted. - for (auto& config_value : values) { - if (config_value->config == default_config) { - return true; - } - } - return false; + return !values.empty() && values.front()->config == ConfigDescription::DefaultConfig(); } ResourceTable::CollisionResult ResourceTable::ResolveFlagCollision(FlagStatus existing, @@ -364,14 +351,14 @@ struct SortedVectorInserter : public Comparer { if (found) { return &*it; } - return &*el.insert(it, std::forward(value)); + return &*el.insert(it, std::move(value)); } }; struct PackageViewComparer { bool operator()(const ResourceTablePackageView& lhs, const ResourceTablePackageView& rhs) { return less_than_struct_with_name_and_id( - lhs, std::make_pair(rhs.name, rhs.id)); + lhs, std::tie(rhs.name, rhs.id)); } }; @@ -384,7 +371,7 @@ struct TypeViewComparer { struct EntryViewComparer { bool operator()(const ResourceTableEntryView& lhs, const ResourceTableEntryView& rhs) { return less_than_struct_with_name_and_id( - lhs, std::make_pair(rhs.name, rhs.id)); + lhs, std::tie(rhs.name, rhs.id)); } }; @@ -429,10 +416,10 @@ void InsertEntryIntoTableView(ResourceTableView& table, const ResourceTablePacka const ResourceConfigValue* ResourceTableEntryView::FindValue(const ConfigDescription& config, android::StringPiece product) const { auto iter = std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product}, - lt_config_key_ref); + lt_config_key_ref()); if (iter != values.end()) { const ResourceConfigValue* value = *iter; - if (value->config == config && StringPiece(value->product) == product) { + if (value->config == config && value->product == product) { return value; } } @@ -615,11 +602,15 @@ bool ResourceTable::AddResource(NewResource&& res, android::IDiagnostics* diag) result = ResolveValueCollision(config_value->value.get(), res.value.get()); } switch (result) { - case CollisionResult::kKeepBoth: + case CollisionResult::kKeepBoth: { // Insert the value ignoring for duplicate configurations - entry->values.push_back(util::make_unique(res.config, res.product)); - entry->values.back()->value = std::move(res.value); + auto it = entry->values.insert( + std::lower_bound(entry->values.begin(), entry->values.end(), + ConfigKey{&res.config, res.product}, lt_config_key_ref()), + util::make_unique(res.config, res.product)); + (*it)->value = std::move(res.value); break; + } case CollisionResult::kTakeNew: // Take the incoming value. -- cgit v1.2.3-59-g8ed1b