Merge "AAPT2: Fix merging of styleables the right way" (79758c8e) · Commits · e / os / android_frameworks_base

tools/aapt2/ResourceTable.cpp

+24 −31

Original line number	Diff line number	Diff line
		@@ -201,8 +201,7 @@ std::vector<ResourceConfigValue*> ResourceEntry::findValuesIf(
		}

		/**
		* The default handler for collisions. A return value of -1 means keep the
		* existing value, 0 means fail, and +1 means take the incoming value.
		* The default handler for collisions.
		*
		* Typically, a weak value will be overridden by a strong value. An existing weak
		* value will not be overridden by an incoming weak value.
		@@ -219,45 +218,34 @@ std::vector<ResourceConfigValue*> ResourceEntry::findValuesIf(
		*
		* A DECL will override a USE without error. Two DECLs must match in their format for there to be
		* no error.
		*
		* Styleables: Styleables are not actual resources, but they are treated as such during the
		* compilation phase. Styleables are allowed to override each other, and their definitions merge
		* and accumulate. If both values are Styleables, we just merge them into the existing value.
		*/
		int ResourceTable::resolveValueCollision(Value* existing, Value* incoming) {
		if (Styleable* existingStyleable = valueCast<Styleable>(existing)) {
		if (Styleable* incomingStyleable = valueCast<Styleable>(incoming)) {
		// Styleables get merged.
		existingStyleable->mergeWith(incomingStyleable);
		return -1;
		}
		}

		ResourceTable::CollisionResult ResourceTable::resolveValueCollision(
		Value* existing, Value* incoming) {
		Attribute* existingAttr = valueCast<Attribute>(existing);
		Attribute* incomingAttr = valueCast<Attribute>(incoming);
		if (!incomingAttr) {
		if (incoming->isWeak()) {
		// We're trying to add a weak resource but a resource
		// already exists. Keep the existing.
		return -1;
		return CollisionResult::kKeepOriginal;
		} else if (existing->isWeak()) {
		// Override the weak resource with the new strong resource.
		return 1;
		return CollisionResult::kTakeNew;
		}
		// The existing and incoming values are strong, this is an error
		// if the values are not both attributes.
		return 0;
		return CollisionResult::kConflict;
		}

		if (!existingAttr) {
		if (existing->isWeak()) {
		// The existing value is not an attribute and it is weak,
		// so take the incoming attribute value.
		return 1;
		return CollisionResult::kTakeNew;
		}
		// The existing value is not an attribute and it is strong,
		// so the incoming attribute value is an error.
		return 0;
		return CollisionResult::kConflict;
		}

		assert(incomingAttr && existingAttr);
		@@ -272,20 +260,20 @@ int ResourceTable::resolveValueCollision(Value* existing, Value* incoming) {
		// The two attributes are both DECLs, but they are plain attributes
		// with the same formats.
		// Keep the strongest one.
		return existingAttr->isWeak() ? 1 : -1;
		return existingAttr->isWeak() ? CollisionResult::kTakeNew : CollisionResult::kKeepOriginal;
		}

		if (existingAttr->isWeak() && existingAttr->typeMask == android::ResTable_map::TYPE_ANY) {
		// Any incoming attribute is better than this.
		return 1;
		return CollisionResult::kTakeNew;
		}

		if (incomingAttr->isWeak() && incomingAttr->typeMask == android::ResTable_map::TYPE_ANY) {
		// The incoming attribute may be a USE instead of a DECL.
		// Keep the existing attribute.
		return -1;
		return CollisionResult::kKeepOriginal;
		}
		return 0;
		return CollisionResult::kConflict;
		}

		static constexpr const char* kValidNameChars = "._-";
		@@ -367,7 +355,7 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name,
		const StringPiece& product,
		std::unique_ptr<Value> value,
		const char* validChars,
		const std::function<int(Value,Value)>& conflictResolver,
		const CollisionResolverFunc& conflictResolver,
		IDiagnostics* diag) {
		assert(value && "value can't be nullptr");
		assert(diag && "diagnostics can't be nullptr");
		@@ -431,17 +419,22 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name,
		configValue->value = std::move(value);

		} else {
		int collisionResult = conflictResolver(configValue->value.get(), value.get());
		if (collisionResult > 0) {
		switch (conflictResolver(configValue->value.get(), value.get())) {
		case CollisionResult::kTakeNew:
		// Take the incoming value.
		configValue->value = std::move(value);
		} else if (collisionResult == 0) {
		break;

		case CollisionResult::kConflict:
		diag->error(DiagMessage(value->getSource())
		<< "duplicate value for resource '" << name << "' "
		<< "with config '" << config << "'");
		diag->error(DiagMessage(configValue->value->getSource())
		<< "resource previously defined here");
		return false;

		case CollisionResult::kKeepOriginal:
		break;
		}
		}

tools/aapt2/ResourceTable.h

+11 −6

Original line number	Diff line number	Diff line
		@@ -38,8 +38,8 @@ namespace aapt {

		enum class SymbolState {
		kUndefined,
		kPrivate,
		kPublic,
		kPrivate
		};

		/**
		@@ -185,13 +185,18 @@ class ResourceTable {
		public:
		ResourceTable() = default;

		enum class CollisionResult {
		kKeepOriginal,
		kConflict,
		kTakeNew
		};

		using CollisionResolverFunc = std::function<CollisionResult(Value, Value)>;

		/**
		* When a collision of resources occurs, this method decides which value to keep.
		* Returns -1 if the existing value should be chosen.
		* Returns 0 if the collision can not be resolved (error).
		* Returns 1 if the incoming value should be chosen.
		*/
		static int resolveValueCollision(Value* existing, Value* incoming);
		static CollisionResult resolveValueCollision(Value* existing, Value* incoming);

		bool addResource(const ResourceNameRef& name,
		const ConfigDescription& config,
		@@ -299,7 +304,7 @@ private:
		const StringPiece& product,
		std::unique_ptr<Value> value,
		const char* validChars,
		const std::function<int(Value,Value)>& conflictResolver,
		const CollisionResolverFunc& conflictResolver,
		IDiagnostics* diag);

		bool setSymbolStateImpl(const ResourceNameRef& name,

tools/aapt2/ResourceTable_test.cpp

+0 −33

Original line number	Diff line number	Diff line
		@@ -118,39 +118,6 @@ TEST(ResourceTableTest, OverrideWeakResourceValue) {
		EXPECT_FALSE(attr->isWeak());
		}

		TEST(ResourceTable, MergeStyleables) {
		ResourceTable table;

		ASSERT_TRUE(table.addResource(
		test::parseNameOrDie("android:styleable/Foo"),
		ConfigDescription{}, "",
		test::StyleableBuilder()
		.addItem("android:attr/bar")
		.addItem("android:attr/foo")
		.build(),
		test::getDiagnostics()));

		ASSERT_TRUE(table.addResource(
		test::parseNameOrDie("android:styleable/Foo"),
		ConfigDescription{}, "",
		test::StyleableBuilder()
		.addItem("android:attr/bat")
		.addItem("android:attr/foo")
		.build(),
		test::getDiagnostics()));

		Styleable* styleable = test::getValue<Styleable>(&table, "android:styleable/Foo");
		ASSERT_NE(nullptr, styleable);

		std::vector<Reference> expectedRefs = {
		Reference(test::parseNameOrDie("android:attr/bar")),
		Reference(test::parseNameOrDie("android:attr/bat")),
		Reference(test::parseNameOrDie("android:attr/foo")),
		};

		EXPECT_EQ(expectedRefs, styleable->entries);
		}

		TEST(ResourceTableTest, ProductVaryingValues) {
		ResourceTable table;

tools/aapt2/ResourceValues.cpp

+14 −3

Original line number	Diff line number	Diff line
		@@ -18,7 +18,6 @@
		#include "ResourceUtils.h"
		#include "ResourceValues.h"
		#include "ValueVisitor.h"
		#include "io/File.h"
		#include "util/Util.h"

		#include <algorithm>
		@@ -66,7 +65,7 @@ void RawString::print(std::ostream* out) const {
		out << "(raw string) " << value;
		}

		Reference::Reference() : referenceType(Reference::Type::kResource) {
		Reference::Reference() : referenceType(Type::kResource) {
		}

		Reference::Reference(const ResourceNameRef& n, Type t) :
		@@ -76,6 +75,10 @@ Reference::Reference(const ResourceNameRef& n, Type t) :
		Reference::Reference(const ResourceId& i, Type type) : id(i), referenceType(type) {
		}

		Reference::Reference(const ResourceNameRef& n, const ResourceId& i) :
		name(n.toResourceName()), id(i), referenceType(Type::kResource) {
		}

		bool Reference::equals(const Value* value) const {
		const Reference* other = valueCast<Reference>(value);
		if (!other) {
		@@ -747,8 +750,16 @@ bool operator!=(const Reference& a, const Reference& b) {
		return a.name != b.name \|\| a.id != b.id;
		}

		struct NameOnlyComparator {
		bool operator()(const Reference& a, const Reference& b) const {
		return a.name < b.name;
		}
		};

		void Styleable::mergeWith(Styleable* other) {
		std::set<Reference> references;
		// Compare only names, because some References may already have their IDs assigned
		// (framework IDs that don't change).
		std::set<Reference, NameOnlyComparator> references;
		references.insert(entries.begin(), entries.end());
		references.insert(other->entries.begin(), other->entries.end());
		entries.clear();

tools/aapt2/ResourceValues.h

+1 −0

Original line number	Diff line number	Diff line
		@@ -172,6 +172,7 @@ struct Reference : public BaseItem<Reference> {
		Reference();
		explicit Reference(const ResourceNameRef& n, Type type = Type::kResource);
		explicit Reference(const ResourceId& i, Type type = Type::kResource);
		explicit Reference(const ResourceNameRef& n, const ResourceId& i);

		bool equals(const Value* value) const override;
		bool flatten(android::Res_value* outValue) const override;