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
diff --git a/tools/aapt2/JavaClassGenerator_test.cpp b/tools/aapt2/JavaClassGenerator_test.cpp
index becf99b..cc5e981 100644
--- a/tools/aapt2/JavaClassGenerator_test.cpp
+++ b/tools/aapt2/JavaClassGenerator_test.cpp
@@ -105,11 +105,9 @@
             .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 bfef9d0..44710eb 100644
--- a/tools/aapt2/ResourceParser.cpp
+++ b/tools/aapt2/ResourceParser.cpp
@@ -186,6 +186,7 @@
     Source source;
     ResourceId id;
     SymbolState symbolState = SymbolState::kUndefined;
+    std::u16string comment;
     std::unique_ptr<Value> value;
     std::list<ParsedResource> childResources;
 };
@@ -194,7 +195,11 @@
 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 @@
         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 @@
         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 @@
  * 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 @@
 
     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 @@
                 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 @@
     }
 
     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 34c68d7..2d5a29d 100644
--- a/tools/aapt2/ResourceParser.h
+++ b/tools/aapt2/ResourceParser.h
@@ -65,12 +65,13 @@
                            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 a7e9d39..af6bf67 100644
--- a/tools/aapt2/ResourceParser_test.cpp
+++ b/tools/aapt2/ResourceParser_test.cpp
@@ -379,18 +379,39 @@
 }
 
 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 84674e8..fa4b109 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* 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 @@
 
     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 @@
 
     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 @@
 
     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 @@
     }
 
     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::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 @@
 
     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 @@
 
     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 @@
 
     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 @@
     }
 
     // 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 be90936..980504b 100644
--- a/tools/aapt2/ResourceTable.h
+++ b/tools/aapt2/ResourceTable.h
@@ -47,12 +47,10 @@
 };
 
 /**
- * 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 @@
     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 @@
      * 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 @@
 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 2055a80..42508fe 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 @@
     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 @@
 
     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, 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 ecc5cd2..f312d75 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 @@
     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 0dae091..2629153 100644
--- a/tools/aapt2/ResourceValues.h
+++ b/tools/aapt2/ResourceValues.h
@@ -41,17 +41,42 @@
 	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.
      */
     virtual void accept(RawValueVisitor* visitor) = 0;
@@ -65,6 +90,10 @@
      * Human readable printout of this value.
      */
     virtual void print(std::ostream* out) const = 0;
+
+private:
+    Source mSource;
+    std::u16string mComment;
 };
 
 /**
@@ -80,11 +109,6 @@
  */
 struct Item : public Value {
     /**
-     * An Item is, of course, an Item.
-     */
-    virtual bool isItem() const override;
-
-    /**
      * Clone the Item.
      */
     virtual Item* clone(StringPool* newPool) const override = 0;
diff --git a/tools/aapt2/ValueVisitor.h b/tools/aapt2/ValueVisitor.h
index ee058aa..94042e3 100644
--- a/tools/aapt2/ValueVisitor.h
+++ b/tools/aapt2/ValueVisitor.h
@@ -115,6 +115,18 @@
 };
 
 /**
+ * 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 1b9499d..cff935c 100644
--- a/tools/aapt2/XmlPullParser.cpp
+++ b/tools/aapt2/XmlPullParser.cpp
@@ -97,7 +97,7 @@
 }
 
 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 f7d7a03..a0ce21d 100644
--- a/tools/aapt2/XmlPullParser.h
+++ b/tools/aapt2/XmlPullParser.h
@@ -158,7 +158,6 @@
         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 095552a..47fa2a6 100644
--- a/tools/aapt2/flatten/TableFlattener.cpp
+++ b/tools/aapt2/flatten/TableFlattener.cpp
@@ -292,7 +292,7 @@
     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 @@
             outEntry->flags |= ResTable_entry::FLAG_WEAK;
         }
 
-        if (!entry->value->isItem()) {
+        if (!IsItem) {
             outEntry->flags |= ResTable_entry::FLAG_COMPLEX;
         }
 
@@ -329,8 +329,8 @@
     }
 
     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 @@
                 }
             }
             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 @@
             // 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 0ccafc2..11fcc5d 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 @@
                             // 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 @@
 
                                 // 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 b84f2e0..ad701de 100644
--- a/tools/aapt2/link/Link.cpp
+++ b/tools/aapt2/link/Link.cpp
@@ -266,13 +266,14 @@
                 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 @@
 
                         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 a2f8d19..dbe0c92 100644
--- a/tools/aapt2/link/PrivateAttributeMover_test.cpp
+++ b/tools/aapt2/link/PrivateAttributeMover_test.cpp
@@ -30,13 +30,9 @@
             .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 b4fb996..8c924b5 100644
--- a/tools/aapt2/link/ReferenceLinker.cpp
+++ b/tools/aapt2/link/ReferenceLinker.cpp
@@ -206,13 +206,13 @@
 
                 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 db52546..636c2ba 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 @@
             }
 
             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 @@
                     }
 
                 } 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::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 c96b080..7309396 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 @@
     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 @@
     }
 
     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 0383c44..1b510e7 100644
--- a/tools/aapt2/test/Builders.h
+++ b/tools/aapt2/test/Builders.h
@@ -43,7 +43,7 @@
         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 @@
         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 @@
         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 @@
         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 @@
     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 c7a715e..314c1e8 100644
--- a/tools/aapt2/unflatten/BinaryResourceParser.cpp
+++ b/tools/aapt2/unflatten/BinaryResourceParser.cpp
@@ -422,26 +422,24 @@
         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 @@
             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 4dbef5d..02c4081 100644
--- a/tools/aapt2/unflatten/BinaryResourceParser.h
+++ b/tools/aapt2/unflatten/BinaryResourceParser.h
@@ -66,26 +66,30 @@
     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 0000000..652018e
--- /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 */