From e78fd617ec60139a973a01925fa7adad31febb39 Mon Sep 17 00:00:00 2001
From: Adam Lesinski <adamlesinski@google.com>
Date: Thu, 22 Oct 2015 12:48:43 -0700
Subject: [PATCH] AAPT2: Move comments and source into Value

Values are closely related to where they were defined, so
this information should live inside the Value.

This also enables comments to be attached to nested Values.

Change-Id: Ic7481b5a5f26d0ef248d638e2e29252f88154581
---
 tools/aapt2/JavaClassGenerator_test.cpp       |  6 +-
 tools/aapt2/ResourceParser.cpp                | 30 ++++--
 tools/aapt2/ResourceParser.h                  | 11 ++-
 tools/aapt2/ResourceParser_test.cpp           | 37 +++++--
 tools/aapt2/ResourceTable.cpp                 | 97 +++++++++----------
 tools/aapt2/ResourceTable.h                   | 37 +++----
 tools/aapt2/ResourceTable_test.cpp            | 50 ++++++----
 tools/aapt2/ResourceValues.cpp                | 13 +--
 tools/aapt2/ResourceValues.h                  | 44 +++++++--
 tools/aapt2/ValueVisitor.h                    | 12 +++
 tools/aapt2/XmlPullParser.cpp                 |  2 +-
 tools/aapt2/XmlPullParser.h                   |  1 -
 tools/aapt2/flatten/TableFlattener.cpp        | 38 +++++---
 tools/aapt2/link/AutoVersioner.cpp            | 32 +++---
 tools/aapt2/link/Link.cpp                     | 22 +++--
 .../aapt2/link/PrivateAttributeMover_test.cpp |  8 +-
 tools/aapt2/link/ReferenceLinker.cpp          |  4 +-
 tools/aapt2/link/TableMerger.cpp              | 34 +++----
 tools/aapt2/process/SymbolTable.cpp           | 24 ++---
 tools/aapt2/test/Builders.h                   | 56 +++++++++--
 .../aapt2/unflatten/BinaryResourceParser.cpp  | 23 ++---
 tools/aapt2/unflatten/BinaryResourceParser.h  | 24 +++--
 tools/aapt2/util/Comparators.h                | 30 ++++++
 23 files changed, 386 insertions(+), 249 deletions(-)
 create mode 100644 tools/aapt2/util/Comparators.h

diff --git a/tools/aapt2/JavaClassGenerator_test.cpp b/tools/aapt2/JavaClassGenerator_test.cpp
index becf99b521ed4..cc5e98150ae3f 100644
--- a/tools/aapt2/JavaClassGenerator_test.cpp
+++ b/tools/aapt2/JavaClassGenerator_test.cpp
@@ -105,11 +105,9 @@ TEST(JavaClassGeneratorTest, OnlyWritePublicResources) {
             .addSimple(u"@android:id/one", ResourceId(0x01020000))
             .addSimple(u"@android:id/two", ResourceId(0x01020001))
             .addSimple(u"@android:id/three", ResourceId(0x01020002))
+            .setSymbolState(u"@android:id/one", ResourceId(0x01020000), SymbolState::kPublic)
+            .setSymbolState(u"@android:id/two", ResourceId(0x01020001), SymbolState::kPrivate)
             .build();
-    ASSERT_TRUE(table->setSymbolState(test::parseNameOrDie(u"@android:id/one"), {}, {},
-                                      SymbolState::kPublic, &diag));
-    ASSERT_TRUE(table->setSymbolState(test::parseNameOrDie(u"@android:id/two"), {}, {},
-                                      SymbolState::kPrivate, &diag));
 
     JavaClassGeneratorOptions options;
     options.types = JavaClassGeneratorOptions::SymbolTypes::kPublic;
diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp
index bfef9d06d7424..44710ebc9dc44 100644
--- a/tools/aapt2/ResourceParser.cpp
+++ b/tools/aapt2/ResourceParser.cpp
@@ -186,6 +186,7 @@ struct ParsedResource {
     Source source;
     ResourceId id;
     SymbolState symbolState = SymbolState::kUndefined;
+    std::u16string comment;
     std::unique_ptr<Value> value;
     std::list<ParsedResource> childResources;
 };
@@ -194,7 +195,11 @@ struct ParsedResource {
 static bool addResourcesToTable(ResourceTable* table, const ConfigDescription& config,
                                 IDiagnostics* diag, ParsedResource* res) {
     if (res->symbolState != SymbolState::kUndefined) {
-        if (!table->setSymbolState(res->name, res->id, res->source, res->symbolState, diag)) {
+        Symbol symbol;
+        symbol.state = res->symbolState;
+        symbol.source = res->source;
+        symbol.comment = res->comment;
+        if (!table->setSymbolState(res->name, res->id, symbol, diag)) {
             return false;
         }
     }
@@ -203,7 +208,11 @@ static bool addResourcesToTable(ResourceTable* table, const ConfigDescription& c
         return true;
     }
 
-    if (!table->addResource(res->name, res->id, config, res->source, std::move(res->value), diag)) {
+    // Attach the comment, source and config to the value.
+    res->value->setComment(std::move(res->comment));
+    res->value->setSource(std::move(res->source));
+
+    if (!table->addResource(res->name, res->id, config, std::move(res->value), diag)) {
         return false;
     }
 
@@ -275,6 +284,7 @@ bool ResourceParser::parseResources(XmlPullParser* parser) {
         ParsedResource parsedResource;
         parsedResource.name.entry = maybeName.value().toString();
         parsedResource.source = mSource.withLine(parser->getLineNumber());
+        parsedResource.comment = std::move(comment);
 
         bool result = true;
         if (elementName == u"id") {
@@ -368,8 +378,8 @@ enum {
  * an Item. If allowRawValue is false, nullptr is returned in this
  * case.
  */
-std::unique_ptr<Item> ResourceParser::parseXml(XmlPullParser* parser, uint32_t typeMask,
-                                               bool allowRawValue) {
+std::unique_ptr<Item> ResourceParser::parseXml(XmlPullParser* parser, const uint32_t typeMask,
+                                               const bool allowRawValue) {
     const size_t beginXmlLine = parser->getLineNumber();
 
     std::u16string rawValue;
@@ -386,8 +396,9 @@ std::unique_ptr<Item> ResourceParser::parseXml(XmlPullParser* parser, uint32_t t
 
     auto onCreateReference = [&](const ResourceName& name) {
         // name.package can be empty here, as it will assume the package name of the table.
-        mTable->addResource(name, {}, mSource.withLine(beginXmlLine), util::make_unique<Id>(),
-                            mDiag);
+        std::unique_ptr<Id> id = util::make_unique<Id>();
+        id->setSource(mSource.withLine(beginXmlLine));
+        mTable->addResource(name, {}, std::move(id), mDiag);
     };
 
     // Process the raw value.
@@ -411,11 +422,12 @@ std::unique_ptr<Item> ResourceParser::parseXml(XmlPullParser* parser, uint32_t t
                 mTable->stringPool.makeRef(styleString.str, StringPool::Context{ 1, mConfig }));
     }
 
-    // We can't parse this so return a RawString if we are allowed.
     if (allowRawValue) {
+        // We can't parse this so return a RawString if we are allowed.
         return util::make_unique<RawString>(
                 mTable->stringPool.makeRef(rawValue, StringPool::Context{ 1, mConfig }));
     }
+
     return {};
 }
 
@@ -683,8 +695,8 @@ Maybe<Attribute::Symbol> ResourceParser::parseEnumOrFlagItem(XmlPullParser* pars
     }
 
     return Attribute::Symbol{
-        Reference(ResourceName{ {}, ResourceType::kId, maybeName.value().toString() }),
-                val.data };
+            Reference(ResourceName({}, ResourceType::kId, maybeName.value().toString())),
+            val.data };
 }
 
 static Maybe<ResourceName> parseXmlAttributeName(StringPiece16 str) {
diff --git a/tools/aapt2/ResourceParser.h b/tools/aapt2/ResourceParser.h
index 34c68d7257a35..2d5a29df2c491 100644
--- a/tools/aapt2/ResourceParser.h
+++ b/tools/aapt2/ResourceParser.h
@@ -65,12 +65,13 @@ private:
                            StyleString* outStyleString);
 
     /*
-     * Parses the XML subtree and converts it to an Item. The type of Item that can be
-     * parsed is denoted by the `typeMask`. If `allowRawValue` is true and the subtree
-     * can not be parsed as a regular Item, then a RawString is returned. Otherwise
-     * this returns nullptr.
+     * Parses the XML subtree and returns an Item.
+     * The type of Item that can be parsed is denoted by the `typeMask`.
+     * If `allowRawValue` is true and the subtree can not be parsed as a regular Item, then a
+     * RawString is returned. Otherwise this returns false;
      */
-    std::unique_ptr<Item> parseXml(XmlPullParser* parser, uint32_t typeMask, bool allowRawValue);
+    std::unique_ptr<Item> parseXml(XmlPullParser* parser, const uint32_t typeMask,
+                                   const bool allowRawValue);
 
     bool parseResources(XmlPullParser* parser);
     bool parseString(XmlPullParser* parser, ParsedResource* outResource);
diff --git a/tools/aapt2/ResourceParser_test.cpp b/tools/aapt2/ResourceParser_test.cpp
index a7e9d390ef4af..af6bf67c4a3b0 100644
--- a/tools/aapt2/ResourceParser_test.cpp
+++ b/tools/aapt2/ResourceParser_test.cpp
@@ -379,18 +379,39 @@ TEST_F(ResourceParserTest, ParsePlural) {
 }
 
 TEST_F(ResourceParserTest, ParseCommentsWithResource) {
-    std::string input = "<!-- This is a comment -->\n"
+    std::string input = "<!--This is a comment-->\n"
                         "<string name=\"foo\">Hi</string>";
     ASSERT_TRUE(testParse(input));
 
-    Maybe<ResourceTable::SearchResult> result = mTable.findResource(
-            test::parseNameOrDie(u"@string/foo"));
-    AAPT_ASSERT_TRUE(result);
+    String* value = test::getValue<String>(&mTable, u"@string/foo");
+    ASSERT_NE(nullptr, value);
+    EXPECT_EQ(value->getComment(), u"This is a comment");
+}
 
-    ResourceEntry* entry = result.value().entry;
-    ASSERT_NE(entry, nullptr);
-    ASSERT_FALSE(entry->values.empty());
-    EXPECT_EQ(entry->values.front().comment, u"This is a comment");
+TEST_F(ResourceParserTest, DoNotCombineMultipleComments) {
+    std::string input = "<!--One-->\n"
+                        "<!--Two-->\n"
+                        "<string name=\"foo\">Hi</string>";
+
+    ASSERT_TRUE(testParse(input));
+
+    String* value = test::getValue<String>(&mTable, u"@string/foo");
+    ASSERT_NE(nullptr, value);
+    EXPECT_EQ(value->getComment(), u"Two");
+}
+
+TEST_F(ResourceParserTest, IgnoreCommentBeforeEndTag) {
+    std::string input = "<!--One-->\n"
+                        "<string name=\"foo\">\n"
+                        "  Hi\n"
+                        "<!--Two-->\n"
+                        "</string>";
+
+    ASSERT_TRUE(testParse(input));
+
+    String* value = test::getValue<String>(&mTable, u"@string/foo");
+    ASSERT_NE(nullptr, value);
+    EXPECT_EQ(value->getComment(), u"One");
 }
 
 /*
diff --git a/tools/aapt2/ResourceTable.cpp b/tools/aapt2/ResourceTable.cpp
index 84674e841063d..fa4b1094434b7 100644
--- a/tools/aapt2/ResourceTable.cpp
+++ b/tools/aapt2/ResourceTable.cpp
@@ -19,6 +19,8 @@
 #include "ResourceTable.h"
 #include "ResourceValues.h"
 #include "ValueVisitor.h"
+
+#include "util/Comparators.h"
 #include "util/Util.h"
 
 #include <algorithm>
@@ -29,10 +31,6 @@
 
 namespace aapt {
 
-static bool compareConfigs(const ResourceConfigValue& lhs, const ConfigDescription& rhs) {
-    return lhs.config < rhs;
-}
-
 static bool lessThanType(const std::unique_ptr<ResourceTableType>& lhs, ResourceType rhs) {
     return lhs->type < rhs;
 }
@@ -191,52 +189,50 @@ static constexpr const char16_t* kValidNameChars = u"._-";
 static constexpr const char16_t* kValidNameMangledChars = u"._-$";
 
 bool ResourceTable::addResource(const ResourceNameRef& name, const ConfigDescription& config,
-                                const Source& source, std::unique_ptr<Value> value,
-                                IDiagnostics* diag) {
-    return addResourceImpl(name, ResourceId{}, config, source, std::move(value), kValidNameChars,
-                           diag);
+                                std::unique_ptr<Value> value, IDiagnostics* diag) {
+    return addResourceImpl(name, {}, config, std::move(value), kValidNameChars, diag);
 }
 
 bool ResourceTable::addResource(const ResourceNameRef& name, const ResourceId resId,
-                                const ConfigDescription& config, const Source& source,
-                                std::unique_ptr<Value> value, IDiagnostics* diag) {
-    return addResourceImpl(name, resId, config, source, std::move(value), kValidNameChars, diag);
+                                const ConfigDescription& config, std::unique_ptr<Value> value,
+                                IDiagnostics* diag) {
+    return addResourceImpl(name, resId, config, std::move(value), kValidNameChars, diag);
 }
 
 bool ResourceTable::addFileReference(const ResourceNameRef& name, const ConfigDescription& config,
                                      const Source& source, const StringPiece16& path,
                                      IDiagnostics* diag) {
-    return addResourceImpl(name, ResourceId{}, config, source,
-                           util::make_unique<FileReference>(stringPool.makeRef(path)),
-                           kValidNameChars, diag);
+    std::unique_ptr<FileReference> fileRef = util::make_unique<FileReference>(
+            stringPool.makeRef(path));
+    fileRef->setSource(source);
+    return addResourceImpl(name, ResourceId{}, config, std::move(fileRef), kValidNameChars, diag);
 }
 
 bool ResourceTable::addResourceAllowMangled(const ResourceNameRef& name,
                                             const ConfigDescription& config,
-                                            const Source& source,
                                             std::unique_ptr<Value> value,
                                             IDiagnostics* diag) {
-    return addResourceImpl(name, ResourceId{}, config, source, std::move(value),
-                           kValidNameMangledChars, diag);
+    return addResourceImpl(name, ResourceId{}, config, std::move(value), kValidNameMangledChars,
+                           diag);
 }
 
 bool ResourceTable::addResourceAllowMangled(const ResourceNameRef& name,
                                             const ResourceId id,
                                             const ConfigDescription& config,
-                                            const Source& source,
                                             std::unique_ptr<Value> value,
                                             IDiagnostics* diag) {
-    return addResourceImpl(name, id, config, source, std::move(value),
-                           kValidNameMangledChars, diag);
+    return addResourceImpl(name, id, config, std::move(value), kValidNameMangledChars, diag);
 }
 
 bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceId resId,
-                                    const ConfigDescription& config, const Source& source,
-                                    std::unique_ptr<Value> value, const char16_t* validChars,
-                                    IDiagnostics* diag) {
+                                    const ConfigDescription& config, std::unique_ptr<Value> value,
+                                    const char16_t* validChars, IDiagnostics* diag) {
+    assert(value && "value can't be nullptr");
+    assert(diag && "diagnostics can't be nullptr");
+
     auto badCharIter = util::findNonAlphaNumericAndNotInSet(name.entry, validChars);
     if (badCharIter != name.entry.end()) {
-        diag->error(DiagMessage(source)
+        diag->error(DiagMessage(value->getSource())
                     << "resource '"
                     << name
                     << "' has invalid entry name '"
@@ -249,7 +245,7 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceI
 
     ResourceTablePackage* package = findOrCreatePackage(name.package);
     if (resId.isValid() && package->id && package->id.value() != resId.packageId()) {
-        diag->error(DiagMessage(source)
+        diag->error(DiagMessage(value->getSource())
                     << "trying to add resource '"
                     << name
                     << "' with ID "
@@ -263,7 +259,7 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceI
 
     ResourceTableType* type = package->findOrCreateType(name.type);
     if (resId.isValid() && type->id && type->id.value() != resId.typeId()) {
-        diag->error(DiagMessage(source)
+        diag->error(DiagMessage(value->getSource())
                     << "trying to add resource '"
                     << name
                     << "' with ID "
@@ -277,7 +273,7 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceI
 
     ResourceEntry* entry = type->findOrCreateEntry(name.entry);
     if (resId.isValid() && entry->id && entry->id.value() != resId.entryId()) {
-        diag->error(DiagMessage(source)
+        diag->error(DiagMessage(value->getSource())
                     << "trying to add resource '"
                     << name
                     << "' with ID "
@@ -288,20 +284,20 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceI
     }
 
     const auto endIter = entry->values.end();
-    auto iter = std::lower_bound(entry->values.begin(), endIter, config, compareConfigs);
+    auto iter = std::lower_bound(entry->values.begin(), endIter, config, cmp::lessThan);
     if (iter == endIter || iter->config != config) {
         // This resource did not exist before, add it.
-        entry->values.insert(iter, ResourceConfigValue{ config, source, {}, std::move(value) });
+        entry->values.insert(iter, ResourceConfigValue{ config, std::move(value) });
     } else {
         int collisionResult = resolveValueCollision(iter->value.get(), value.get());
         if (collisionResult > 0) {
             // Take the incoming value.
-            *iter = ResourceConfigValue{ config, source, {}, std::move(value) };
+            iter->value = std::move(value);
         } else if (collisionResult == 0) {
-            diag->error(DiagMessage(source)
+            diag->error(DiagMessage(value->getSource())
                         << "duplicate value for resource '" << name << "' "
-                        << "with config '" << iter->config << "'");
-            diag->error(DiagMessage(iter->source)
+                        << "with config '" << config << "'");
+            diag->error(DiagMessage(iter->value->getSource())
                         << "resource previously defined here");
             return false;
         }
@@ -316,27 +312,29 @@ bool ResourceTable::addResourceImpl(const ResourceNameRef& name, const ResourceI
 }
 
 bool ResourceTable::setSymbolState(const ResourceNameRef& name, const ResourceId resId,
-                                   const Source& source, SymbolState state, IDiagnostics* diag) {
-    return setSymbolStateImpl(name, resId, source, state, kValidNameChars, diag);
+                                   const Symbol& symbol, IDiagnostics* diag) {
+    return setSymbolStateImpl(name, resId, symbol, kValidNameChars, diag);
 }
 
-bool ResourceTable::setSymbolStateAllowMangled(const ResourceNameRef& name, const ResourceId resId,
-                                               const Source& source, SymbolState state,
-                                               IDiagnostics* diag) {
-    return setSymbolStateImpl(name, resId, source, state, kValidNameMangledChars, diag);
+bool ResourceTable::setSymbolStateAllowMangled(const ResourceNameRef& name,
+                                               const ResourceId resId,
+                                               const Symbol& symbol, IDiagnostics* diag) {
+    return setSymbolStateImpl(name, resId, symbol, kValidNameMangledChars, diag);
 }
 
 bool ResourceTable::setSymbolStateImpl(const ResourceNameRef& name, const ResourceId resId,
-                                       const Source& source, SymbolState state,
-                                       const char16_t* validChars, IDiagnostics* diag) {
-    if (state == SymbolState::kUndefined) {
+                                       const Symbol& symbol, const char16_t* validChars,
+                                       IDiagnostics* diag) {
+    assert(diag && "diagnostics can't be nullptr");
+
+    if (symbol.state == SymbolState::kUndefined) {
         // Nothing to do.
         return true;
     }
 
     auto badCharIter = util::findNonAlphaNumericAndNotInSet(name.entry, validChars);
     if (badCharIter != name.entry.end()) {
-        diag->error(DiagMessage(source)
+        diag->error(DiagMessage(symbol.source)
                     << "resource '"
                     << name
                     << "' has invalid entry name '"
@@ -349,7 +347,7 @@ bool ResourceTable::setSymbolStateImpl(const ResourceNameRef& name, const Resour
 
     ResourceTablePackage* package = findOrCreatePackage(name.package);
     if (resId.isValid() && package->id && package->id.value() != resId.packageId()) {
-        diag->error(DiagMessage(source)
+        diag->error(DiagMessage(symbol.source)
                     << "trying to add resource '"
                     << name
                     << "' with ID "
@@ -363,7 +361,7 @@ bool ResourceTable::setSymbolStateImpl(const ResourceNameRef& name, const Resour
 
     ResourceTableType* type = package->findOrCreateType(name.type);
     if (resId.isValid() && type->id && type->id.value() != resId.typeId()) {
-        diag->error(DiagMessage(source)
+        diag->error(DiagMessage(symbol.source)
                     << "trying to add resource '"
                     << name
                     << "' with ID "
@@ -377,7 +375,7 @@ bool ResourceTable::setSymbolStateImpl(const ResourceNameRef& name, const Resour
 
     ResourceEntry* entry = type->findOrCreateEntry(name.entry);
     if (resId.isValid() && entry->id && entry->id.value() != resId.entryId()) {
-        diag->error(DiagMessage(source)
+        diag->error(DiagMessage(symbol.source)
                     << "trying to add resource '"
                     << name
                     << "' with ID "
@@ -388,15 +386,14 @@ bool ResourceTable::setSymbolStateImpl(const ResourceNameRef& name, const Resour
     }
 
     // Only mark the type state as public, it doesn't care about being private.
-    if (state == SymbolState::kPublic) {
+    if (symbol.state == SymbolState::kPublic) {
         type->symbolStatus.state = SymbolState::kPublic;
     }
 
     // Downgrading to a private symbol from a public one is not allowed.
     if (entry->symbolStatus.state != SymbolState::kPublic) {
-        if (entry->symbolStatus.state != state) {
-            entry->symbolStatus.state = state;
-            entry->symbolStatus.source = source;
+        if (entry->symbolStatus.state != symbol.state) {
+            entry->symbolStatus = std::move(symbol);
         }
     }
 
diff --git a/tools/aapt2/ResourceTable.h b/tools/aapt2/ResourceTable.h
index be909361bef10..980504be377b2 100644
--- a/tools/aapt2/ResourceTable.h
+++ b/tools/aapt2/ResourceTable.h
@@ -47,12 +47,10 @@ struct Symbol {
 };
 
 /**
- * The resource value for a specific configuration.
+ * Represents a value defined for a given configuration.
  */
 struct ResourceConfigValue {
     ConfigDescription config;
-    Source source;
-    std::u16string comment;
     std::unique_ptr<Value> value;
 };
 
@@ -158,12 +156,11 @@ public:
     static int resolveValueCollision(Value* existing, Value* incoming);
 
     bool addResource(const ResourceNameRef& name, const ConfigDescription& config,
-                     const Source& source, std::unique_ptr<Value> value,
-                     IDiagnostics* diag);
+                     std::unique_ptr<Value> value, IDiagnostics* diag);
 
     bool addResource(const ResourceNameRef& name, const ResourceId resId,
-                     const ConfigDescription& config, const Source& source,
-                     std::unique_ptr<Value> value, IDiagnostics* diag);
+                     const ConfigDescription& config, std::unique_ptr<Value> value,
+                     IDiagnostics* diag);
 
     bool addFileReference(const ResourceNameRef& name, const ConfigDescription& config,
                           const Source& source, const StringPiece16& path, IDiagnostics* diag);
@@ -174,18 +171,18 @@ public:
      * names.
      */
     bool addResourceAllowMangled(const ResourceNameRef& name, const ConfigDescription& config,
-                                 const Source& source, std::unique_ptr<Value> value,
-                                 IDiagnostics* diag);
+                                 std::unique_ptr<Value> value, IDiagnostics* diag);
 
     bool addResourceAllowMangled(const ResourceNameRef& name, const ResourceId id,
-                                 const ConfigDescription& config,
-                                 const Source& source, std::unique_ptr<Value> value,
+                                 const ConfigDescription& config, std::unique_ptr<Value> value,
                                  IDiagnostics* diag);
 
-    bool setSymbolState(const ResourceNameRef& name, const ResourceId resId, const Source& source,
-                        SymbolState state, IDiagnostics* diag);
+    bool setSymbolState(const ResourceNameRef& name, const ResourceId resId,
+                        const Symbol& symbol, IDiagnostics* diag);
+
     bool setSymbolStateAllowMangled(const ResourceNameRef& name, const ResourceId resId,
-                                    const Source& source, SymbolState state, IDiagnostics* diag);
+                                    const Symbol& symbol, IDiagnostics* diag);
+
     struct SearchResult {
         ResourceTablePackage* package;
         ResourceTableType* type;
@@ -224,13 +221,11 @@ public:
 private:
     ResourceTablePackage* findOrCreatePackage(const StringPiece16& name);
 
-    bool addResourceImpl(const ResourceNameRef& name, const ResourceId resId,
-                         const ConfigDescription& config, const Source& source,
-                         std::unique_ptr<Value> value, const char16_t* validChars,
-                         IDiagnostics* diag);
-    bool setSymbolStateImpl(const ResourceNameRef& name, const ResourceId resId,
-                            const Source& source, SymbolState state, const char16_t* validChars,
-                            IDiagnostics* diag);
+    bool addResourceImpl(const ResourceNameRef& name, ResourceId resId,
+                         const ConfigDescription& config, std::unique_ptr<Value> value,
+                         const char16_t* validChars, IDiagnostics* diag);
+    bool setSymbolStateImpl(const ResourceNameRef& name, ResourceId resId,
+                            const Symbol& symbol, const char16_t* validChars, IDiagnostics* diag);
 };
 
 } // namespace aapt
diff --git a/tools/aapt2/ResourceTable_test.cpp b/tools/aapt2/ResourceTable_test.cpp
index 2055a80aaba6c..42508fe154b8d 100644
--- a/tools/aapt2/ResourceTable_test.cpp
+++ b/tools/aapt2/ResourceTable_test.cpp
@@ -19,7 +19,7 @@
 #include "ResourceValues.h"
 #include "util/Util.h"
 
-#include "test/Common.h"
+#include "test/Builders.h"
 
 #include <algorithm>
 #include <gtest/gtest.h>
@@ -42,22 +42,26 @@ TEST_F(ResourceTableTest, FailToAddResourceWithBadName) {
     ResourceTable table;
 
     EXPECT_FALSE(table.addResource(
-            ResourceNameRef{ u"android", ResourceType::kId, u"hey,there" },
-            {}, Source{ "test.xml", 21 },
-            util::make_unique<Id>(), &mDiagnostics));
+            ResourceNameRef(u"android", ResourceType::kId, u"hey,there"),
+            ConfigDescription{},
+            test::ValueBuilder<Id>().setSource("test.xml", 21u).build(),
+            &mDiagnostics));
 
     EXPECT_FALSE(table.addResource(
-            ResourceNameRef{ u"android", ResourceType::kId, u"hey:there" },
-            {}, Source{ "test.xml", 21 },
-            util::make_unique<Id>(), &mDiagnostics));
+            ResourceNameRef(u"android", ResourceType::kId, u"hey:there"),
+            ConfigDescription{},
+            test::ValueBuilder<Id>().setSource("test.xml", 21u).build(),
+            &mDiagnostics));
 }
 
 TEST_F(ResourceTableTest, AddOneResource) {
     ResourceTable table;
 
-    EXPECT_TRUE(table.addResource(test::parseNameOrDie(u"@android:attr/id"), {},
-                                  Source{ "test/path/file.xml", 23 },
-                                  util::make_unique<Id>(), &mDiagnostics));
+    EXPECT_TRUE(table.addResource(test::parseNameOrDie(u"@android:attr/id"),
+                                  ConfigDescription{},
+                                  test::ValueBuilder<Id>()
+                                          .setSource("test/path/file.xml", 23u).build(),
+                                  &mDiagnostics));
 
     ASSERT_NE(nullptr, test::getValue<Id>(&table, u"@android:attr/id"));
 }
@@ -71,23 +75,29 @@ TEST_F(ResourceTableTest, AddMultipleResources) {
 
     EXPECT_TRUE(table.addResource(
             test::parseNameOrDie(u"@android:attr/layout_width"),
-            config, Source{ "test/path/file.xml", 10 },
-            util::make_unique<Id>(), &mDiagnostics));
+            config,
+            test::ValueBuilder<Id>().setSource("test/path/file.xml", 10u).build(),
+            &mDiagnostics));
 
     EXPECT_TRUE(table.addResource(
             test::parseNameOrDie(u"@android:attr/id"),
-            config, Source{ "test/path/file.xml", 12 },
-            util::make_unique<Id>(), &mDiagnostics));
+            config,
+            test::ValueBuilder<Id>().setSource("test/path/file.xml", 12u).build(),
+            &mDiagnostics));
 
     EXPECT_TRUE(table.addResource(
             test::parseNameOrDie(u"@android:string/ok"),
-            config, Source{ "test/path/file.xml", 14 },
-            util::make_unique<Id>(), &mDiagnostics));
+            config,
+            test::ValueBuilder<Id>().setSource("test/path/file.xml", 14u).build(),
+            &mDiagnostics));
 
     EXPECT_TRUE(table.addResource(
             test::parseNameOrDie(u"@android:string/ok"),
-            languageConfig, Source{ "test/path/file.xml", 20 },
-            util::make_unique<BinaryPrimitive>(android::Res_value{}), &mDiagnostics));
+            languageConfig,
+            test::ValueBuilder<BinaryPrimitive>(android::Res_value{})
+                    .setSource("test/path/file.xml", 20u)
+                    .build(),
+            &mDiagnostics));
 
     ASSERT_NE(nullptr, test::getValue<Id>(&table, u"@android:attr/layout_width"));
     ASSERT_NE(nullptr, test::getValue<Id>(&table, u"@android:attr/id"));
@@ -99,14 +109,14 @@ TEST_F(ResourceTableTest, AddMultipleResources) {
 TEST_F(ResourceTableTest, OverrideWeakResourceValue) {
     ResourceTable table;
 
-    ASSERT_TRUE(table.addResource(test::parseNameOrDie(u"@android:attr/foo"), {}, {},
+    ASSERT_TRUE(table.addResource(test::parseNameOrDie(u"@android:attr/foo"), ConfigDescription{},
                                   util::make_unique<Attribute>(true), &mDiagnostics));
 
     Attribute* attr = test::getValue<Attribute>(&table, u"@android:attr/foo");
     ASSERT_NE(nullptr, attr);
     EXPECT_TRUE(attr->isWeak());
 
-    ASSERT_TRUE(table.addResource(test::parseNameOrDie(u"@android:attr/foo"), {}, {},
+    ASSERT_TRUE(table.addResource(test::parseNameOrDie(u"@android:attr/foo"), ConfigDescription{},
                                   util::make_unique<Attribute>(false), &mDiagnostics));
 
     attr = test::getValue<Attribute>(&table, u"@android:attr/foo");
diff --git a/tools/aapt2/ResourceValues.cpp b/tools/aapt2/ResourceValues.cpp
index ecc5cd2bdcfab..f312d75ab1e9b 100644
--- a/tools/aapt2/ResourceValues.cpp
+++ b/tools/aapt2/ResourceValues.cpp
@@ -15,11 +15,12 @@
  */
 
 #include "Resource.h"
-#include "flatten/ResourceTypeExtensions.h"
 #include "ResourceValues.h"
-#include "util/Util.h"
 #include "ValueVisitor.h"
 
+#include "util/Util.h"
+#include "flatten/ResourceTypeExtensions.h"
+
 #include <androidfw/ResourceTypes.h>
 #include <limits>
 
@@ -35,18 +36,10 @@ void BaseItem<Derived>::accept(RawValueVisitor* visitor) {
     visitor->visit(static_cast<Derived*>(this));
 }
 
-bool Value::isItem() const {
-    return false;
-}
-
 bool Value::isWeak() const {
     return false;
 }
 
-bool Item::isItem() const {
-    return true;
-}
-
 RawString::RawString(const StringPool::Ref& ref) : value(ref) {
 }
 
diff --git a/tools/aapt2/ResourceValues.h b/tools/aapt2/ResourceValues.h
index 0dae091dffb43..26291536ae80f 100644
--- a/tools/aapt2/ResourceValues.h
+++ b/tools/aapt2/ResourceValues.h
@@ -40,17 +40,42 @@ struct RawValueVisitor;
 struct Value {
 	virtual ~Value() = default;
 
-    /**
-     * Whether or not this is an Item.
-     */
-    virtual bool isItem() const;
-
     /**
      * Whether this value is weak and can be overridden without
      * warning or error. Default for base class is false.
      */
     virtual bool isWeak() const;
 
+    /**
+     * Returns the source where this value was defined.
+     */
+    const Source& getSource() const {
+        return mSource;
+    }
+
+    void setSource(const Source& source) {
+        mSource = source;
+    }
+
+    void setSource(Source&& source) {
+        mSource = std::move(source);
+    }
+
+    /**
+     * Returns the comment that was associated with this resource.
+     */
+    StringPiece16 getComment() const {
+        return mComment;
+    }
+
+    void setComment(const StringPiece16& str) {
+        mComment = str.toString();
+    }
+
+    void setComment(std::u16string&& str) {
+        mComment = std::move(str);
+    }
+
     /**
      * Calls the appropriate overload of ValueVisitor.
      */
@@ -65,6 +90,10 @@ struct Value {
      * Human readable printout of this value.
      */
     virtual void print(std::ostream* out) const = 0;
+
+private:
+    Source mSource;
+    std::u16string mComment;
 };
 
 /**
@@ -79,11 +108,6 @@ struct BaseValue : public Value {
  * A resource item with a single value. This maps to android::ResTable_entry.
  */
 struct Item : public Value {
-    /**
-     * An Item is, of course, an Item.
-     */
-    virtual bool isItem() const override;
-
     /**
      * Clone the Item.
      */
diff --git a/tools/aapt2/ValueVisitor.h b/tools/aapt2/ValueVisitor.h
index ee058aa1a37b6..94042e3c2618c 100644
--- a/tools/aapt2/ValueVisitor.h
+++ b/tools/aapt2/ValueVisitor.h
@@ -114,6 +114,18 @@ struct DynCastVisitor : public RawValueVisitor {
     }
 };
 
+/**
+ * Specialization that checks if the value is an Item.
+ */
+template <>
+struct DynCastVisitor<Item> : public RawValueVisitor {
+    Item* value = nullptr;
+
+    void visitItem(Item* item) override {
+        value = item;
+    }
+};
+
 /**
  * Returns a valid pointer to T if the Value is of subtype T.
  * Otherwise, returns nullptr.
diff --git a/tools/aapt2/XmlPullParser.cpp b/tools/aapt2/XmlPullParser.cpp
index 1b9499d78efb9..cff935c10aaef 100644
--- a/tools/aapt2/XmlPullParser.cpp
+++ b/tools/aapt2/XmlPullParser.cpp
@@ -97,7 +97,7 @@ const std::string& XmlPullParser::getLastError() const {
 }
 
 const std::u16string& XmlPullParser::getComment() const {
-    return mEventQueue.front().comment;
+    return mEventQueue.front().data1;
 }
 
 size_t XmlPullParser::getLineNumber() const {
diff --git a/tools/aapt2/XmlPullParser.h b/tools/aapt2/XmlPullParser.h
index f7d7a03a4352b..a0ce21dd3218f 100644
--- a/tools/aapt2/XmlPullParser.h
+++ b/tools/aapt2/XmlPullParser.h
@@ -158,7 +158,6 @@ private:
         size_t depth;
         std::u16string data1;
         std::u16string data2;
-        std::u16string comment;
         std::vector<Attribute> attributes;
     };
 
diff --git a/tools/aapt2/flatten/TableFlattener.cpp b/tools/aapt2/flatten/TableFlattener.cpp
index 095552aa7f316..47fa2a63d5533 100644
--- a/tools/aapt2/flatten/TableFlattener.cpp
+++ b/tools/aapt2/flatten/TableFlattener.cpp
@@ -292,7 +292,7 @@ private:
     SymbolWriter* mSymbols;
     StringPool* mSourcePool;
 
-    template <typename T>
+    template <typename T, bool IsItem>
     T* writeEntry(FlatEntry* entry, BigBuffer* buffer) {
         static_assert(std::is_same<ResTable_entry, T>::value ||
                       std::is_same<ResTable_entry_ext, T>::value,
@@ -308,7 +308,7 @@ private:
             outEntry->flags |= ResTable_entry::FLAG_WEAK;
         }
 
-        if (!entry->value->isItem()) {
+        if (!IsItem) {
             outEntry->flags |= ResTable_entry::FLAG_COMPLEX;
         }
 
@@ -329,8 +329,8 @@ private:
     }
 
     bool flattenValue(FlatEntry* entry, BigBuffer* buffer) {
-        if (entry->value->isItem()) {
-            writeEntry<ResTable_entry>(entry, buffer);
+        if (Item* item = valueCast<Item>(entry->value)) {
+            writeEntry<ResTable_entry, true>(entry, buffer);
             if (Reference* ref = valueCast<Reference>(entry->value)) {
                 if (!ref->id) {
                     assert(ref->name && "reference must have at least a name");
@@ -339,12 +339,12 @@ private:
                 }
             }
             Res_value* outValue = buffer->nextBlock<Res_value>();
-            bool result = static_cast<Item*>(entry->value)->flatten(outValue);
+            bool result = item->flatten(outValue);
             assert(result && "flatten failed");
             outValue->size = util::hostToDevice16(sizeof(*outValue));
         } else {
             const size_t beforeEntry = buffer->size();
-            ResTable_entry_ext* outEntry = writeEntry<ResTable_entry_ext>(entry, buffer);
+            ResTable_entry_ext* outEntry = writeEntry<ResTable_entry_ext, false>(entry, buffer);
             MapFlattenVisitor visitor(mSymbols, entry, buffer);
             entry->value->accept(&visitor);
             outEntry->count = util::hostToDevice32(visitor.mEntryCount);
@@ -551,17 +551,27 @@ private:
             // configuration available. Here we reverse this to match the binary table.
             std::map<ConfigDescription, std::vector<FlatEntry>> configToEntryListMap;
             for (ResourceEntry* entry : sortedEntries) {
-                const size_t keyIndex = mKeyPool.makeRef(entry->name).getIndex();
+                const uint32_t keyIndex = (uint32_t) mKeyPool.makeRef(entry->name).getIndex();
 
                 // Group values by configuration.
                 for (auto& configValue : entry->values) {
-                   configToEntryListMap[configValue.config].push_back(FlatEntry{
-                            entry, configValue.value.get(), (uint32_t) keyIndex,
-                            (uint32_t)(mSourcePool->makeRef(util::utf8ToUtf16(
-                                    configValue.source.path)).getIndex()),
-                            (uint32_t)(configValue.source.line
-                                    ? configValue.source.line.value() : 0)
-                   });
+                    Value* value = configValue.value.get();
+
+                    const StringPool::Ref sourceRef = mSourcePool->makeRef(
+                            util::utf8ToUtf16(value->getSource().path));
+
+                    uint32_t lineNumber = 0;
+                    if (value->getSource().line) {
+                        lineNumber = value->getSource().line.value();
+                    }
+
+                    configToEntryListMap[configValue.config]
+                            .push_back(FlatEntry{
+                                    entry,
+                                    value,
+                                    keyIndex,
+                                    (uint32_t) sourceRef.getIndex(),
+                                    lineNumber });
                 }
             }
 
diff --git a/tools/aapt2/link/AutoVersioner.cpp b/tools/aapt2/link/AutoVersioner.cpp
index 0ccafc2107d01..11fcc5d6274d9 100644
--- a/tools/aapt2/link/AutoVersioner.cpp
+++ b/tools/aapt2/link/AutoVersioner.cpp
@@ -20,21 +20,18 @@
 #include "ValueVisitor.h"
 
 #include "link/Linkers.h"
+#include "util/Comparators.h"
 
 #include <algorithm>
 #include <cassert>
 
 namespace aapt {
 
-static bool cmpConfigValue(const ResourceConfigValue& lhs, const ConfigDescription& config) {
-    return lhs.config < config;
-}
-
 bool shouldGenerateVersionedResource(const ResourceEntry* entry, const ConfigDescription& config,
                                      const int sdkVersionToGenerate) {
     assert(sdkVersionToGenerate > config.sdkVersion);
     const auto endIter = entry->values.end();
-    auto iter = std::lower_bound(entry->values.begin(), endIter, config, cmpConfigValue);
+    auto iter = std::lower_bound(entry->values.begin(), endIter, config, cmp::lessThan);
 
     // The source config came from this list, so it should be here.
     assert(iter != entry->values.end());
@@ -107,21 +104,16 @@ bool AutoVersioner::consume(IAaptContext* context, ResourceTable* table) {
                             // We found attributes from a higher SDK level. Check that
                             // there is no other defined resource for the version we want to
                             // generate.
-                            if (shouldGenerateVersionedResource(entry.get(), configValue.config,
+                            if (shouldGenerateVersionedResource(entry.get(),
+                                                                configValue.config,
                                                                 minSdkStripped.value())) {
                                 // Let's create a new Style for this versioned resource.
                                 ConfigDescription newConfig(configValue.config);
                                 newConfig.sdkVersion = minSdkStripped.value();
 
-                                ResourceConfigValue newValue = {
-                                        newConfig,
-                                        configValue.source,
-                                        configValue.comment,
-                                        std::unique_ptr<Value>(configValue.value->clone(
-                                                &table->stringPool))
-                                };
-
-                                Style* newStyle = static_cast<Style*>(newValue.value.get());
+                                std::unique_ptr<Style> newStyle(style->clone(&table->stringPool));
+                                newStyle->setComment(style->getComment());
+                                newStyle->setSource(style->getSource());
 
                                 // Move the previously stripped attributes into this style.
                                 newStyle->entries.insert(newStyle->entries.end(),
@@ -130,9 +122,13 @@ bool AutoVersioner::consume(IAaptContext* context, ResourceTable* table) {
 
                                 // Insert the new Resource into the correct place.
                                 auto iter = std::lower_bound(entry->values.begin(),
-                                                             entry->values.end(), newConfig,
-                                                             cmpConfigValue);
-                                entry->values.insert(iter, std::move(newValue));
+                                                             entry->values.end(),
+                                                             newConfig,
+                                                             cmp::lessThan);
+
+                                entry->values.insert(
+                                        iter,
+                                        ResourceConfigValue{ newConfig, std::move(newStyle) });
                             }
                         }
                     }
diff --git a/tools/aapt2/link/Link.cpp b/tools/aapt2/link/Link.cpp
index b84f2e068dc27..ad701deac16f8 100644
--- a/tools/aapt2/link/Link.cpp
+++ b/tools/aapt2/link/Link.cpp
@@ -266,13 +266,14 @@ struct LinkCommand {
                 for (const auto& type : package->types) {
                     for (const auto& entry : type->entries) {
                         for (const auto& configValue : entry->values) {
-                            mContext.getDiagnostics()->error(DiagMessage(configValue.source)
-                                                             << "defined resource '"
-                                                             << ResourceNameRef(package->name,
-                                                                                type->type,
-                                                                                entry->name)
-                                                             << "' for external package '"
-                                                             << package->name << "'");
+                            mContext.getDiagnostics()->error(
+                                    DiagMessage(configValue.value->getSource())
+                                                << "defined resource '"
+                                                << ResourceNameRef(package->name,
+                                                                   type->type,
+                                                                   entry->name)
+                                                << "' for external package '"
+                                                << package->name << "'");
                             error = true;
                         }
                     }
@@ -472,10 +473,11 @@ struct LinkCommand {
 
                         Maybe<ResourceName> mangledName = mContext.getNameMangler()->mangleName(
                                 exportedSymbol.name);
-                        if (!mergedTable.addResource(
+                        std::unique_ptr<Id> id = util::make_unique<Id>();
+                        id->setSource(f.source.withLine(exportedSymbol.line));
+                        if (!mergedTable.addResourceAllowMangled(
                                 mangledName ? mangledName.value() : exportedSymbol.name,
-                                {}, {}, f.source.withLine(exportedSymbol.line),
-                                util::make_unique<Id>(), mContext.getDiagnostics())) {
+                                {}, std::move(id), mContext.getDiagnostics())) {
                             error = true;
                         }
                     }
diff --git a/tools/aapt2/link/PrivateAttributeMover_test.cpp b/tools/aapt2/link/PrivateAttributeMover_test.cpp
index a2f8d19b57cca..dbe0c92253c1e 100644
--- a/tools/aapt2/link/PrivateAttributeMover_test.cpp
+++ b/tools/aapt2/link/PrivateAttributeMover_test.cpp
@@ -30,13 +30,9 @@ TEST(PrivateAttributeMoverTest, MovePrivateAttributes) {
             .addSimple(u"@android:attr/privateA")
             .addSimple(u"@android:attr/publicB")
             .addSimple(u"@android:attr/privateB")
+            .setSymbolState(u"@android:attr/publicA", ResourceId(0x01010000), SymbolState::kPublic)
+            .setSymbolState(u"@android:attr/publicB", ResourceId(0x01010000), SymbolState::kPublic)
             .build();
-    ASSERT_TRUE(table->setSymbolState(test::parseNameOrDie(u"@android:attr/publicA"),
-                                      ResourceId(0x01010000), {}, SymbolState::kPublic,
-                                      context->getDiagnostics()));
-    ASSERT_TRUE(table->setSymbolState(test::parseNameOrDie(u"@android:attr/publicB"),
-                                      ResourceId(0x01010002), {}, SymbolState::kPublic,
-                                      context->getDiagnostics()));
 
     PrivateAttributeMover mover;
     ASSERT_TRUE(mover.consume(context.get(), table.get()));
diff --git a/tools/aapt2/link/ReferenceLinker.cpp b/tools/aapt2/link/ReferenceLinker.cpp
index b4fb9961f0c54..8c924b5b0f99b 100644
--- a/tools/aapt2/link/ReferenceLinker.cpp
+++ b/tools/aapt2/link/ReferenceLinker.cpp
@@ -206,13 +206,13 @@ public:
 
                 if (!(typeMask & ResourceUtils::androidTypeToAttributeTypeMask(val.dataType))) {
                     // The actual type of this item is incompatible with the attribute.
-                    DiagMessage msg;
+                    DiagMessage msg(style->getSource());
                     buildAttributeMismatchMessage(&msg, s->attribute.get(), entry.value.get());
                     mContext->getDiagnostics()->error(msg);
                     mError = true;
                 }
             } else {
-                DiagMessage msg;
+                DiagMessage msg(style->getSource());
                 msg << "style attribute '";
                 if (entry.key.name) {
                     msg << entry.key.name.value().package << ":" << entry.key.name.value().entry;
diff --git a/tools/aapt2/link/TableMerger.cpp b/tools/aapt2/link/TableMerger.cpp
index db525467933f7..636c2ba9e9fd6 100644
--- a/tools/aapt2/link/TableMerger.cpp
+++ b/tools/aapt2/link/TableMerger.cpp
@@ -19,6 +19,7 @@
 #include "ValueVisitor.h"
 
 #include "link/TableMerger.h"
+#include "util/Comparators.h"
 #include "util/Util.h"
 
 #include <cassert>
@@ -120,27 +121,24 @@ bool TableMerger::doMerge(const Source& src, ResourceTable* srcTable,
             }
 
             for (ResourceConfigValue& srcValue : srcEntry->values) {
-                auto cmp = [](const ResourceConfigValue& a,
-                              const ConfigDescription& b) -> bool {
-                    return a.config < b;
-                };
-
                 auto iter = std::lower_bound(dstEntry->values.begin(), dstEntry->values.end(),
-                                             srcValue.config, cmp);
+                                             srcValue.config, cmp::lessThan);
 
                 if (iter != dstEntry->values.end() && iter->config == srcValue.config) {
                     const int collisionResult = ResourceTable::resolveValueCollision(
                             iter->value.get(), srcValue.value.get());
                     if (collisionResult == 0) {
                         // Error!
-                        ResourceNameRef resourceName =
-                                { srcPackage->name, srcType->type, srcEntry->name };
-                        mContext->getDiagnostics()->error(DiagMessage(srcValue.source)
+                        ResourceNameRef resourceName(srcPackage->name,
+                                                     srcType->type,
+                                                     srcEntry->name);
+
+                        mContext->getDiagnostics()->error(DiagMessage(srcValue.value->getSource())
                                                           << "resource '" << resourceName
                                                           << "' has a conflicting value for "
                                                           << "configuration ("
                                                           << srcValue.config << ")");
-                        mContext->getDiagnostics()->note(DiagMessage(iter->source)
+                        mContext->getDiagnostics()->note(DiagMessage(iter->value->getSource())
                                                          << "originally defined here");
                         error = true;
                         continue;
@@ -150,16 +148,12 @@ bool TableMerger::doMerge(const Source& src, ResourceTable* srcTable,
                     }
 
                 } else {
-                    // Insert a new value.
-                    iter = dstEntry->values.insert(iter,
-                                                   ResourceConfigValue{ srcValue.config });
+                    // Insert a place holder value. We will fill it in below.
+                    iter = dstEntry->values.insert(iter, ResourceConfigValue{ srcValue.config });
                 }
 
-                iter->source = std::move(srcValue.source);
-                iter->comment = std::move(srcValue.comment);
                 if (manglePackage) {
-                    iter->value = cloneAndMangle(srcTable, srcPackage->name,
-                                                 srcValue.value.get());
+                    iter->value = cloneAndMangle(srcTable, srcPackage->name, srcValue.value.get());
                 } else {
                     iter->value = clone(srcValue.value.get());
                 }
@@ -179,7 +173,11 @@ std::unique_ptr<Value> TableMerger::cloneAndMangle(ResourceTable* table,
             std::u16string mangledEntry = NameMangler::mangleEntry(package, entry.toString());
             std::u16string newPath = prefix.toString() + mangledEntry + suffix.toString();
             mFilesToMerge.push(FileToMerge{ table, *f->path, newPath });
-            return util::make_unique<FileReference>(mMasterTable->stringPool.makeRef(newPath));
+            std::unique_ptr<FileReference> fileRef = util::make_unique<FileReference>(
+                    mMasterTable->stringPool.makeRef(newPath));
+            fileRef->setComment(f->getComment());
+            fileRef->setSource(f->getSource());
+            return std::move(fileRef);
         }
     }
     return clone(value);
diff --git a/tools/aapt2/process/SymbolTable.cpp b/tools/aapt2/process/SymbolTable.cpp
index c96b080f586b8..7309396547e6a 100644
--- a/tools/aapt2/process/SymbolTable.cpp
+++ b/tools/aapt2/process/SymbolTable.cpp
@@ -16,9 +16,11 @@
 
 #include "ConfigDescription.h"
 #include "Resource.h"
-#include "util/Util.h"
+#include "ValueVisitor.h"
 
 #include "process/SymbolTable.h"
+#include "util/Comparators.h"
+#include "util/Util.h"
 
 #include <androidfw/AssetManager.h>
 #include <androidfw/ResourceTypes.h>
@@ -34,7 +36,7 @@ const ISymbolTable::Symbol* SymbolTableWrapper::findByName(const ResourceName& n
     if (!result) {
         if (name.type == ResourceType::kAttr) {
             // Recurse and try looking up a private attribute.
-            return findByName(ResourceName{ name.package, ResourceType::kAttrPrivate, name.entry });
+            return findByName(ResourceName(name.package, ResourceType::kAttrPrivate, name.entry));
         }
         return {};
     }
@@ -48,28 +50,26 @@ const ISymbolTable::Symbol* SymbolTableWrapper::findByName(const ResourceName& n
     }
 
     std::shared_ptr<Symbol> symbol = std::make_shared<Symbol>();
-    symbol->id = ResourceId{
-            sr.package->id.value(), sr.type->id.value(), sr.entry->id.value() };
+    symbol->id = ResourceId(sr.package->id.value(), sr.type->id.value(), sr.entry->id.value());
 
     if (name.type == ResourceType::kAttr || name.type == ResourceType::kAttrPrivate) {
-        auto lt = [](ResourceConfigValue& lhs, const ConfigDescription& rhs) -> bool {
-            return lhs.config < rhs;
-        };
-
         const ConfigDescription kDefaultConfig;
         auto iter = std::lower_bound(sr.entry->values.begin(), sr.entry->values.end(),
-                                     kDefaultConfig, lt);
+                                     kDefaultConfig, cmp::lessThan);
 
         if (iter != sr.entry->values.end() && iter->config == kDefaultConfig) {
             // This resource has an Attribute.
-            symbol->attribute = util::make_unique<Attribute>(
-                    *static_cast<Attribute*>(iter->value.get()));
+            if (Attribute* attr = valueCast<Attribute>(iter->value.get())) {
+                symbol->attribute = std::unique_ptr<Attribute>(attr->clone(nullptr));
+            } else {
+                return {};
+            }
         }
     }
 
     if (name.type == ResourceType::kAttrPrivate) {
         // Masquerade this entry as kAttr.
-        mCache.put(ResourceName{ name.package, ResourceType::kAttr, name.entry }, symbol);
+        mCache.put(ResourceName(name.package, ResourceType::kAttr, name.entry), symbol);
     } else {
         mCache.put(name, symbol);
     }
diff --git a/tools/aapt2/test/Builders.h b/tools/aapt2/test/Builders.h
index 0383c443d4c10..1b510e756855a 100644
--- a/tools/aapt2/test/Builders.h
+++ b/tools/aapt2/test/Builders.h
@@ -43,7 +43,7 @@ public:
         return *this;
     }
 
-    ResourceTableBuilder& addSimple(const StringPiece16& name, ResourceId id = {}) {
+    ResourceTableBuilder& addSimple(const StringPiece16& name, const ResourceId id = {}) {
         return addValue(name, id, util::make_unique<Id>());
     }
 
@@ -51,7 +51,7 @@ public:
         return addReference(name, {}, ref);
     }
 
-    ResourceTableBuilder& addReference(const StringPiece16& name, ResourceId id,
+    ResourceTableBuilder& addReference(const StringPiece16& name, const ResourceId id,
                                        const StringPiece16& ref) {
         return addValue(name, id, util::make_unique<Reference>(parseNameOrDie(ref)));
     }
@@ -60,7 +60,7 @@ public:
         return addString(name, {}, str);
     }
 
-    ResourceTableBuilder& addString(const StringPiece16& name, ResourceId id,
+    ResourceTableBuilder& addString(const StringPiece16& name, const ResourceId id,
                                     const StringPiece16& str) {
         return addValue(name, id, util::make_unique<String>(mTable->stringPool.makeRef(str)));
     }
@@ -69,31 +69,43 @@ public:
         return addFileReference(name, {}, path);
     }
 
-    ResourceTableBuilder& addFileReference(const StringPiece16& name, ResourceId id,
+    ResourceTableBuilder& addFileReference(const StringPiece16& name, const ResourceId id,
                                            const StringPiece16& path) {
         return addValue(name, id,
                         util::make_unique<FileReference>(mTable->stringPool.makeRef(path)));
     }
 
 
-    ResourceTableBuilder& addValue(const StringPiece16& name, std::unique_ptr<Value> value) {
+    ResourceTableBuilder& addValue(const StringPiece16& name,
+                                   std::unique_ptr<Value> value) {
         return addValue(name, {}, std::move(value));
     }
 
-    ResourceTableBuilder& addValue(const StringPiece16& name, ResourceId id,
+    ResourceTableBuilder& addValue(const StringPiece16& name, const ResourceId id,
                                        std::unique_ptr<Value> value) {
         return addValue(name, id, {}, std::move(value));
     }
 
-    ResourceTableBuilder& addValue(const StringPiece16& name, ResourceId id,
-                                   const ConfigDescription& config, std::unique_ptr<Value> value) {
+    ResourceTableBuilder& addValue(const StringPiece16& name, const ResourceId id,
+                                   const ConfigDescription& config,
+                                   std::unique_ptr<Value> value) {
         ResourceName resName = parseNameOrDie(name);
-        bool result = mTable->addResourceAllowMangled(resName, id, config, {}, std::move(value),
+        bool result = mTable->addResourceAllowMangled(resName, id, config, std::move(value),
                                                       &mDiagnostics);
         assert(result);
         return *this;
     }
 
+    ResourceTableBuilder& setSymbolState(const StringPiece16& name, ResourceId id,
+                                         SymbolState state) {
+        ResourceName resName = parseNameOrDie(name);
+        Symbol symbol;
+        symbol.state = state;
+        bool result = mTable->setSymbolStateAllowMangled(resName, id, symbol, &mDiagnostics);
+        assert(result);
+        return *this;
+    }
+
     std::unique_ptr<ResourceTable> build() {
         return std::move(mTable);
     }
@@ -106,6 +118,32 @@ inline std::unique_ptr<Reference> buildReference(const StringPiece16& ref,
     return reference;
 }
 
+template <typename T>
+class ValueBuilder {
+private:
+    std::unique_ptr<Value> mValue;
+
+public:
+    template <typename... Args>
+    ValueBuilder(Args&&... args) : mValue(new T{ std::forward<Args>(args)... }) {
+    }
+
+    template <typename... Args>
+    ValueBuilder& setSource(Args&&... args) {
+        mValue->setSource(Source{ std::forward<Args>(args)... });
+        return *this;
+    }
+
+    ValueBuilder& setComment(const StringPiece16& str) {
+        mValue->setComment(str);
+        return *this;
+    }
+
+    std::unique_ptr<Value> build() {
+        return std::move(mValue);
+    }
+};
+
 class AttributeBuilder {
 private:
     std::unique_ptr<Attribute> mAttr;
diff --git a/tools/aapt2/unflatten/BinaryResourceParser.cpp b/tools/aapt2/unflatten/BinaryResourceParser.cpp
index c7a715e7d2e27..314c1e84f44fb 100644
--- a/tools/aapt2/unflatten/BinaryResourceParser.cpp
+++ b/tools/aapt2/unflatten/BinaryResourceParser.cpp
@@ -422,26 +422,24 @@ bool BinaryResourceParser::parsePublic(const ResourceTablePackage* package,
         const ResourceName name(package->name, *parsedType,
                                 util::getString(mKeyPool, entry->key.index).toString());
 
-        Source source;
+        Symbol symbol;
         if (mSourcePool.getError() == NO_ERROR) {
-            source.path = util::utf16ToUtf8(util::getString(
+            symbol.source.path = util::utf16ToUtf8(util::getString(
                     mSourcePool, util::deviceToHost32(entry->source.index)));
-            source.line = util::deviceToHost32(entry->sourceLine);
+            symbol.source.line = util::deviceToHost32(entry->sourceLine);
         }
 
-        SymbolState state = SymbolState::kUndefined;
         switch (util::deviceToHost16(entry->state)) {
         case Public_entry::kPrivate:
-            state = SymbolState::kPrivate;
+            symbol.state = SymbolState::kPrivate;
             break;
 
         case Public_entry::kPublic:
-            state = SymbolState::kPublic;
+            symbol.state = SymbolState::kPublic;
             break;
         }
 
-        if (!mTable->setSymbolStateAllowMangled(name, resId, source, state,
-                                                mContext->getDiagnostics())) {
+        if (!mTable->setSymbolStateAllowMangled(name, resId, symbol, mContext->getDiagnostics())) {
             return false;
         }
 
@@ -570,14 +568,17 @@ bool BinaryResourceParser::parseType(const ResourceTablePackage* package,
             source.line = util::deviceToHost32(sourceBlock->line);
         }
 
-        if (!mTable->addResourceAllowMangled(name, config, source, std::move(resourceValue),
+        resourceValue->setSource(source);
+        if (!mTable->addResourceAllowMangled(name, config, std::move(resourceValue),
                                              mContext->getDiagnostics())) {
             return false;
         }
 
         if ((entry->flags & ResTable_entry::FLAG_PUBLIC) != 0) {
-            if (!mTable->setSymbolStateAllowMangled(name, resId, mSource.withLine(0),
-                                                    SymbolState::kPublic,
+            Symbol symbol;
+            symbol.state = SymbolState::kPublic;
+            symbol.source = mSource.withLine(0);
+            if (!mTable->setSymbolStateAllowMangled(name, resId, symbol,
                                                     mContext->getDiagnostics())) {
                 return false;
             }
diff --git a/tools/aapt2/unflatten/BinaryResourceParser.h b/tools/aapt2/unflatten/BinaryResourceParser.h
index 4dbef5dcb8154..02c4081cc0e26 100644
--- a/tools/aapt2/unflatten/BinaryResourceParser.h
+++ b/tools/aapt2/unflatten/BinaryResourceParser.h
@@ -66,26 +66,30 @@ private:
     bool parseTypeSpec(const android::ResChunk_header* chunk);
     bool parseType(const ResourceTablePackage* package, const android::ResChunk_header* chunk);
 
-    std::unique_ptr<Item> parseValue(const ResourceNameRef& name,
-            const ConfigDescription& config, const android::Res_value* value, uint16_t flags);
+    std::unique_ptr<Item> parseValue(const ResourceNameRef& name, const ConfigDescription& config,
+                                     const android::Res_value* value, uint16_t flags);
 
     std::unique_ptr<Value> parseMapEntry(const ResourceNameRef& name,
-            const ConfigDescription& config, const android::ResTable_map_entry* map);
+                                         const ConfigDescription& config,
+                                         const android::ResTable_map_entry* map);
 
-    std::unique_ptr<Style> parseStyle(const ResourceNameRef& name,
-            const ConfigDescription& config, const android::ResTable_map_entry* map);
+    std::unique_ptr<Style> parseStyle(const ResourceNameRef& name, const ConfigDescription& config,
+                                      const android::ResTable_map_entry* map);
 
     std::unique_ptr<Attribute> parseAttr(const ResourceNameRef& name,
-            const ConfigDescription& config, const android::ResTable_map_entry* map);
+                                         const ConfigDescription& config,
+                                         const android::ResTable_map_entry* map);
 
-    std::unique_ptr<Array> parseArray(const ResourceNameRef& name,
-            const ConfigDescription& config, const android::ResTable_map_entry* map);
+    std::unique_ptr<Array> parseArray(const ResourceNameRef& name, const ConfigDescription& config,
+                                      const android::ResTable_map_entry* map);
 
     std::unique_ptr<Plural> parsePlural(const ResourceNameRef& name,
-            const ConfigDescription& config, const android::ResTable_map_entry* map);
+                                        const ConfigDescription& config,
+                                        const android::ResTable_map_entry* map);
 
     std::unique_ptr<Styleable> parseStyleable(const ResourceNameRef& name,
-            const ConfigDescription& config, const android::ResTable_map_entry* map);
+                                              const ConfigDescription& config,
+                                              const android::ResTable_map_entry* map);
 
     IAaptContext* mContext;
     ResourceTable* mTable;
diff --git a/tools/aapt2/util/Comparators.h b/tools/aapt2/util/Comparators.h
new file mode 100644
index 0000000000000..652018e342c7a
--- /dev/null
+++ b/tools/aapt2/util/Comparators.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed 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.
+ */
+
+#ifndef AAPT_UTIL_COMPARATORS_H
+#define AAPT_UTIL_COMPARATORS_H
+
+namespace aapt {
+namespace cmp {
+
+inline bool lessThan(const ResourceConfigValue& a, const ConfigDescription& b) {
+    return a.config < b;
+}
+
+} // namespace cmp
+} // namespace aapt
+
+#endif /* AAPT_UTIL_COMPARATORS_H */