Merge changes I851f9fb9,I60dbcb24 into sc-dev

* changes:
  Refactor aapt2 tests ResourceTable changes
  Prepare aapt2 for multiple ids per type

tools/aapt2/Debug.cpp

@@ -256,53 +256,35 @@ class ValueBodyPrinter : public ConstValueVisitor {
 
 void Debug::PrintTable(const ResourceTable& table, const DebugPrintTableOptions& options,
                        Printer* printer) {
-  for (const auto& package : table.packages) {
+  const auto table_view = table.GetPartitionedView();
-    ValueHeadlinePrinter headline_printer(package->name, printer);
+  for (const auto& package : table_view.packages) {
-    ValueBodyPrinter body_printer(package->name, printer);
+    ValueHeadlinePrinter headline_printer(package.name, printer);
+    ValueBodyPrinter body_printer(package.name, printer);
 
     printer->Print("Package name=");
-    printer->Print(package->name);
+    printer->Print(package.name);
-    if (package->id) {
+    if (package.id) {
-      printer->Print(StringPrintf(" id=%02x", package->id.value()));
+      printer->Print(StringPrintf(" id=%02x", package.id.value()));
     }
     printer->Println();
 
     printer->Indent();
-    for (const auto& type : package->types) {
+    for (const auto& type : package.types) {
       printer->Print("type ");
-      printer->Print(to_string(type->type));
+      printer->Print(to_string(type.type));
-      if (type->id) {
+      if (type.id) {
-        printer->Print(StringPrintf(" id=%02x", type->id.value()));
+        printer->Print(StringPrintf(" id=%02x", type.id.value()));
-      }
-      printer->Println(StringPrintf(" entryCount=%zd", type->entries.size()));
-
-      std::vector<const ResourceEntry*> sorted_entries;
-      for (const auto& entry : type->entries) {
-        auto iter = std::lower_bound(
-            sorted_entries.begin(), sorted_entries.end(), entry.get(),
-            [](const ResourceEntry* a, const ResourceEntry* b) -> bool {
-              if (a->id && b->id) {
-                return a->id.value() < b->id.value();
-              } else if (a->id) {
-                return true;
-              } else {
-                return false;
-              }
-            });
-        sorted_entries.insert(iter, entry.get());
       }
+      printer->Println(StringPrintf(" entryCount=%zd", type.entries.size()));
 
       printer->Indent();
-      for (const ResourceEntry* entry : sorted_entries) {
+      for (const ResourceEntry* entry : type.entries) {
-        const ResourceId id(package->id.value_or_default(0), type->id.value_or_default(0),
-                            entry->id.value_or_default(0));
-
         printer->Print("resource ");
-        printer->Print(id.to_string());
+        printer->Print(entry->id.value_or_default(0).to_string());
         printer->Print(" ");
 
         // Write the name without the package (this is obvious and too verbose).
-        printer->Print(to_string(type->type));
+        printer->Print(to_string(type.type));
         printer->Print("/");
         printer->Print(entry->name);
 

tools/aapt2/LoadedApk.cpp

@@ -113,7 +113,7 @@ std::unique_ptr<LoadedApk> LoadedApk::LoadProtoApkFromFileCollection(
     }
 
     std::string error;
-    table = util::make_unique<ResourceTable>(/** validate_resources **/ false);
+    table = util::make_unique<ResourceTable>(ResourceTable::Validation::kDisabled);
     if (!DeserializeTableFromPb(pb_table, collection.get(), table.get(), &error)) {
       diag->Error(DiagMessage(source)
                   << "failed to deserialize " << kProtoResourceTablePath << ": " << error);
@@ -157,7 +157,7 @@ std::unique_ptr<LoadedApk> LoadedApk::LoadBinaryApkFromFileCollection(
 
   io::IFile* table_file = collection->FindFile(kApkResourceTablePath);
   if (table_file != nullptr) {
-    table = util::make_unique<ResourceTable>(/** validate_resources **/ false);
+    table = util::make_unique<ResourceTable>(ResourceTable::Validation::kDisabled);
     std::unique_ptr<io::IData> data = table_file->OpenAsData();
     if (data == nullptr) {
       diag->Error(DiagMessage(source) << "failed to open " << kApkResourceTablePath);

tools/aapt2/Resource.h

@@ -138,7 +138,7 @@ struct ResourceId {
   uint32_t id;
 
   ResourceId();
-  ResourceId(const ResourceId& rhs);
+  ResourceId(const ResourceId& rhs) = default;
   ResourceId(uint32_t res_id);  // NOLINT(google-explicit-constructor)
   ResourceId(uint8_t p, uint8_t t, uint16_t e);
 
@@ -222,8 +222,6 @@ bool operator<(const ResourceKeyRef& a, const ResourceKeyRef& b);
 
 inline ResourceId::ResourceId() : id(0) {}
 
-inline ResourceId::ResourceId(const ResourceId& rhs) : id(rhs.id) {}
-
 inline ResourceId::ResourceId(uint32_t res_id) : id(res_id) {}
 
 inline ResourceId::ResourceId(uint8_t p, uint8_t t, uint16_t e)

tools/aapt2/ResourceParser.cpp

@@ -118,43 +118,43 @@ static bool AddResourcesToTable(ResourceTable* table, IDiagnostics* diag, Parsed
     res->comment = trimmed_comment.to_string();
   }
 
+  NewResourceBuilder res_builder(res->name);
   if (res->visibility_level != Visibility::Level::kUndefined) {
     Visibility visibility;
     visibility.level = res->visibility_level;
     visibility.source = res->source;
     visibility.comment = res->comment;
-    if (!table->SetVisibilityWithId(res->name, visibility, res->id, diag)) {
+    res_builder.SetVisibility(visibility);
-      return false;
   }
+
+  if (res->id.is_valid()) {
+    res_builder.SetId(res->id);
   }
 
   if (res->allow_new) {
     AllowNew allow_new;
     allow_new.source = res->source;
     allow_new.comment = res->comment;
-    if (!table->SetAllowNew(res->name, allow_new, diag)) {
+    res_builder.SetAllowNew(allow_new);
-      return false;
-    }
   }
 
   if (res->overlayable_item) {
-    if (!table->SetOverlayable(res->name, res->overlayable_item.value(), diag)) {
+    res_builder.SetOverlayable(res->overlayable_item.value());
-      return false;
-    }
   }
 
   if (res->value != nullptr) {
     // Attach the comment, source and config to the value.
     res->value->SetComment(std::move(res->comment));
     res->value->SetSource(std::move(res->source));
-
+    res_builder.SetValue(std::move(res->value), res->config, res->product);
-    if (!table->AddResourceWithId(res->name, res->id, res->config, res->product,
-                                  std::move(res->value), diag)) {
-      return false;
-    }
   }
 
   bool error = false;
+  if (!res->name.entry.empty()) {
+    if (!table->AddResource(res_builder.Build(), diag)) {
+      return false;
+    }
+  }
   for (ParsedResource& child : res->child_resources) {
     error |= !AddResourcesToTable(table, diag, &child);
   }
@@ -751,7 +751,7 @@ std::unique_ptr<Item> ResourceParser::ParseXml(xml::XmlPullParser* parser,
     // table.
     std::unique_ptr<Id> id = util::make_unique<Id>();
     id->SetSource(source_.WithLine(begin_xml_line));
-    table_->AddResource(name, {}, {}, std::move(id), diag_);
+    table_->AddResource(NewResourceBuilder(name).SetValue(std::move(id)).Build(), diag_);
   };
 
   // Process the raw value.

tools/aapt2/ResourceParser_test.cpp

@@ -831,25 +831,13 @@ TEST_F(ResourceParserTest, AutoIncrementIdsInPublicGroup) {
 
   Maybe<ResourceTable::SearchResult> result = table_.FindResource(test::ParseNameOrDie("attr/foo"));
   ASSERT_TRUE(result);
-
-  ASSERT_TRUE(result.value().package->id);
-  ASSERT_TRUE(result.value().type->id);
   ASSERT_TRUE(result.value().entry->id);
-  ResourceId actual_id(result.value().package->id.value(),
+  EXPECT_THAT(result.value().entry->id.value(), Eq(ResourceId(0x01010040)));
-                       result.value().type->id.value(),
-                       result.value().entry->id.value());
-  EXPECT_THAT(actual_id, Eq(ResourceId(0x01010040)));
 
   result = table_.FindResource(test::ParseNameOrDie("attr/bar"));
   ASSERT_TRUE(result);
-
-  ASSERT_TRUE(result.value().package->id);
-  ASSERT_TRUE(result.value().type->id);
   ASSERT_TRUE(result.value().entry->id);
-  actual_id = ResourceId(result.value().package->id.value(),
+  EXPECT_THAT(result.value().entry->id.value(), Eq(ResourceId(0x01010041)));
-                         result.value().type->id.value(),
-                         result.value().entry->id.value());
-  EXPECT_THAT(actual_id, Eq(ResourceId(0x01010041)));
 }
 
 TEST_F(ResourceParserTest, StrongestSymbolVisibilityWins) {

tools/aapt2/ResourceTable.cpp

@@ -18,20 +18,18 @@
 
 #include <algorithm>
 #include <memory>
-#include <string>
 #include <tuple>
 
 #include "android-base/logging.h"
-#include "android-base/stringprintf.h"
 #include "androidfw/ConfigDescription.h"
 #include "androidfw/ResourceTypes.h"
 
-#include "Debug.h"
 #include "NameMangler.h"
+#include "ResourceUtils.h"
 #include "ResourceValues.h"
 #include "ValueVisitor.h"
-#include "trace/TraceBuffer.h"
 #include "text/Unicode.h"
+#include "trace/TraceBuffer.h"
 #include "util/Util.h"
 
 using ::aapt::text::IsValidResourceEntryName;
@@ -43,135 +41,108 @@ namespace aapt {
 
 const char* Overlayable::kActorScheme = "overlay";
 
-static bool less_than_type_and_id(const std::unique_ptr<ResourceTableType>& lhs,
+namespace {
-                                  const std::pair<ResourceType, Maybe<uint8_t>>& rhs) {
+bool less_than_type(const std::unique_ptr<ResourceTableType>& lhs, ResourceType rhs) {
-  return lhs->type < rhs.first || (lhs->type == rhs.first && rhs.second && lhs->id < rhs.second);
+  return lhs->type < rhs;
 }
 
 template <typename T>
-static bool less_than_struct_with_name(const std::unique_ptr<T>& lhs, const StringPiece& rhs) {
+bool less_than_type_and_id(const T& lhs, const std::pair<ResourceType, Maybe<uint8_t>>& rhs) {
+  return lhs.id != rhs.second ? lhs.id < rhs.second : lhs.type < rhs.first;
+}
+
+template <typename T>
+bool less_than_struct_with_name(const std::unique_ptr<T>& lhs, const StringPiece& rhs) {
   return lhs->name.compare(0, lhs->name.size(), rhs.data(), rhs.size()) < 0;
 }
 
 template <typename T>
-static bool less_than_struct_with_name_and_id(const std::unique_ptr<T>& lhs,
+bool greater_than_struct_with_name(const StringPiece& lhs, const std::unique_ptr<T>& rhs) {
-                                              const std::pair<StringPiece, Maybe<uint16_t>>& rhs) {
+  return rhs->name.compare(0, rhs->name.size(), lhs.data(), lhs.size()) > 0;
-  int name_cmp = lhs->name.compare(0, lhs->name.size(), rhs.first.data(), rhs.first.size());
-  return name_cmp < 0 || (name_cmp == 0 && rhs.second && lhs->id < rhs.second);
 }
 
-ResourceTablePackage* ResourceTable::FindPackage(const StringPiece& name) const {
+template <typename T>
-  const auto last = packages.end();
+struct NameEqualRange {
-  auto iter = std::lower_bound(packages.begin(), last, name,
+  bool operator()(const std::unique_ptr<T>& lhs, const StringPiece& rhs) const {
-                               less_than_struct_with_name<ResourceTablePackage>);
+    return less_than_struct_with_name<T>(lhs, rhs);
-  if (iter != last && name == (*iter)->name) {
-    return iter->get();
   }
-  return nullptr;
+  bool operator()(const StringPiece& lhs, const std::unique_ptr<T>& rhs) const {
+    return greater_than_struct_with_name<T>(lhs, rhs);
+  }
+};
+
+template <typename T, typename U>
+bool less_than_struct_with_name_and_id(const T& lhs,
+                                       const std::pair<std::string_view, Maybe<U>>& rhs) {
+  if (lhs.id != rhs.second) {
+    return lhs.id < rhs.second;
+  }
+  return lhs.name.compare(0, lhs.name.size(), rhs.first.data(), rhs.first.size()) < 0;
 }
 
-ResourceTablePackage* ResourceTable::FindPackageById(uint8_t id) const {
+template <typename T, typename U>
-  for (auto& package : packages) {
+bool less_than_struct_with_name_and_id_pointer(const T* lhs,
-    if (package->id && package->id.value() == id) {
+                                               const std::pair<std::string_view, Maybe<U>>& rhs) {
-      return package.get();
+  return less_than_struct_with_name_and_id(*lhs, rhs);
-    }
-  }
-  return nullptr;
 }
 
-ResourceTablePackage* ResourceTable::CreatePackage(const StringPiece& name, Maybe<uint8_t> id) {
+template <typename T, typename Func, typename Elements>
-  TRACE_CALL();
+T* FindElementsRunAction(const android::StringPiece& name, Elements& entries, Func action) {
-  ResourceTablePackage* package = FindOrCreatePackage(name);
+  const auto iter =
-  if (id && !package->id) {
+      std::lower_bound(entries.begin(), entries.end(), name, less_than_struct_with_name<T>);
-    package->id = id;
+  const bool found = iter != entries.end() && name == (*iter)->name;
-    return package;
+  return action(found, iter);
 }
 
-  if (id && package->id && package->id.value() != id.value()) {
+}  // namespace
-    return nullptr;
+
-  }
+ResourceTable::ResourceTable(ResourceTable::Validation validation) : validation_(validation) {
-  return package;
 }
 
-ResourceTablePackage* ResourceTable::CreatePackageAllowingDuplicateNames(const StringPiece& name,
+ResourceTablePackage* ResourceTable::FindPackage(const android::StringPiece& name) const {
-                                                                         const Maybe<uint8_t> id) {
+  return FindElementsRunAction<ResourceTablePackage>(
-  const auto last = packages.end();
+      name, packages, [&](bool found, auto& iter) { return found ? iter->get() : nullptr; });
-  auto iter = std::lower_bound(packages.begin(), last, std::make_pair(name, id),
-                               less_than_struct_with_name_and_id<ResourceTablePackage>);
-
-  if (iter != last && name == (*iter)->name && id == (*iter)->id) {
-    return iter->get();
 }
 
-  std::unique_ptr<ResourceTablePackage> new_package = util::make_unique<ResourceTablePackage>();
+ResourceTablePackage* ResourceTable::FindOrCreatePackage(const android::StringPiece& name) {
-  new_package->name = name.to_string();
+  return FindElementsRunAction<ResourceTablePackage>(name, packages, [&](bool found, auto& iter) {
-  new_package->id = id;
+    return found ? iter->get() : packages.emplace(iter, new ResourceTablePackage(name))->get();
-  return packages.emplace(iter, std::move(new_package))->get();
+  });
 }
 
-ResourceTablePackage* ResourceTable::FindOrCreatePackage(const StringPiece& name) {
+template <typename Func, typename Elements>
-  const auto last = packages.end();
+static ResourceTableType* FindTypeRunAction(ResourceType type, Elements& entries, Func action) {
-  auto iter = std::lower_bound(packages.begin(), last, name,
+  const auto iter = std::lower_bound(entries.begin(), entries.end(), type, less_than_type);
-                               less_than_struct_with_name<ResourceTablePackage>);
+  const bool found = iter != entries.end() && type == (*iter)->type;
-  if (iter != last && name == (*iter)->name) {
+  return action(found, iter);
-    return iter->get();
 }
 
-  std::unique_ptr<ResourceTablePackage> new_package = util::make_unique<ResourceTablePackage>();
+ResourceTableType* ResourceTablePackage::FindType(ResourceType type) const {
-  new_package->name = name.to_string();
+  return FindTypeRunAction(type, types,
-  return packages.emplace(iter, std::move(new_package))->get();
+                           [&](bool found, auto& iter) { return found ? iter->get() : nullptr; });
 }
 
-ResourceTableType* ResourceTablePackage::FindType(ResourceType type, const Maybe<uint8_t> id) {
+ResourceTableType* ResourceTablePackage::FindOrCreateType(ResourceType type) {
-  const auto last = types.end();
+  return FindTypeRunAction(type, types, [&](bool found, auto& iter) {
-  auto iter = std::lower_bound(types.begin(), last, std::make_pair(type, id),
+    return found ? iter->get() : types.emplace(iter, new ResourceTableType(type))->get();
-                               less_than_type_and_id);
+  });
-  if (iter != last && (*iter)->type == type && (!id || id == (*iter)->id)) {
-    return iter->get();
-  }
-  return nullptr;
 }
 
-ResourceTableType* ResourceTablePackage::FindOrCreateType(ResourceType type,
+ResourceEntry* ResourceTableType::CreateEntry(const android::StringPiece& name) {
-                                                          const Maybe<uint8_t> id) {
+  return FindElementsRunAction<ResourceEntry>(name, entries, [&](bool found, auto& iter) {
-  const auto last = types.end();
+    return entries.emplace(iter, new ResourceEntry(name))->get();
-  auto iter = std::lower_bound(types.begin(), last, std::make_pair(type, id),
+  });
-                               less_than_type_and_id);
-  if (iter != last && (*iter)->type == type && (!id || id == (*iter)->id)) {
-    return iter->get();
 }
 
-  auto new_type = new ResourceTableType(type);
+ResourceEntry* ResourceTableType::FindEntry(const android::StringPiece& name) const {
-  new_type->id = id;
+  return FindElementsRunAction<ResourceEntry>(
-  return types.emplace(iter, std::move(new_type))->get();
+      name, entries, [&](bool found, auto& iter) { return found ? iter->get() : nullptr; });
 }
 
-ResourceEntry* ResourceTableType::FindEntry(const StringPiece& name, const Maybe<uint16_t> id) {
+ResourceEntry* ResourceTableType::FindOrCreateEntry(const android::StringPiece& name) {
-  const auto last = entries.end();
+  return FindElementsRunAction<ResourceEntry>(name, entries, [&](bool found, auto& iter) {
-  auto iter = std::lower_bound(entries.begin(), last, std::make_pair(name, id),
+    return found ? iter->get() : entries.emplace(iter, new ResourceEntry(name))->get();
-      less_than_struct_with_name_and_id<ResourceEntry>);
+  });
-  if (iter != last && name == (*iter)->name && (!id || id == (*iter)->id)) {
-    return iter->get();
-  }
-  return nullptr;
-}
-
-ResourceEntry* ResourceTableType::FindOrCreateEntry(const StringPiece& name,
-                                                    const Maybe<uint16_t > id) {
-  auto last = entries.end();
-  auto iter = std::lower_bound(entries.begin(), last, std::make_pair(name, id),
-                               less_than_struct_with_name_and_id<ResourceEntry>);
-  if (iter != last && name == (*iter)->name && (!id || id == (*iter)->id)) {
-    return iter->get();
-  }
-
-  auto new_entry = new ResourceEntry(name);
-  new_entry->id = id;
-  return entries.emplace(iter, std::move(new_entry))->get();
-}
-
-ResourceConfigValue* ResourceEntry::FindValue(const ConfigDescription& config) {
-  return FindValue(config, StringPiece());
 }
 
 struct ConfigKey {
@@ -188,7 +159,20 @@ bool lt_config_key_ref(const std::unique_ptr<ResourceConfigValue>& lhs, const Co
 }
 
 ResourceConfigValue* ResourceEntry::FindValue(const ConfigDescription& config,
-                                              const StringPiece& product) {
+                                              android::StringPiece product) {
+  auto iter = std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product},
+                               lt_config_key_ref);
+  if (iter != values.end()) {
+    ResourceConfigValue* value = iter->get();
+    if (value->config == config && StringPiece(value->product) == product) {
+      return value;
+    }
+  }
+  return nullptr;
+}
+
+const ResourceConfigValue* ResourceEntry::FindValue(const android::ConfigDescription& config,
+                                                    android::StringPiece product) const {
   auto iter = std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product},
                                lt_config_key_ref);
   if (iter != values.end()) {
@@ -323,138 +307,165 @@ ResourceTable::CollisionResult ResourceTable::ResolveValueCollision(Value* exist
   return CollisionResult::kConflict;
 }
 
-ResourceTable::CollisionResult ResourceTable::IgnoreCollision(Value* /** existing **/,
+ResourceTableView ResourceTable::GetPartitionedView() const {
-                                                              Value* /** incoming **/) {
+  ResourceTableView view;
-  return CollisionResult::kKeepBoth;
+  for (const auto& package : packages) {
+    for (const auto& type : package->types) {
+      for (const auto& entry : type->entries) {
+        std::pair<std::string_view, Maybe<uint8_t>> package_key(package->name, {});
+        std::pair<std::string_view, Maybe<ResourceId>> entry_key(entry->name, {});
+        std::pair<ResourceType, Maybe<uint8_t>> type_key(type->type, {});
+        if (entry->id) {
+          // If the entry has a defined id, use the id to determine insertion position.
+          package_key.second = entry->id.value().package_id();
+          type_key.second = entry->id.value().type_id();
+          entry_key.second = entry->id.value();
         }
 
-static StringPiece ResourceNameValidator(const StringPiece& name) {
+        auto package_it =
-  if (!IsValidResourceEntryName(name)) {
+            std::lower_bound(view.packages.begin(), view.packages.end(), package_key,
-    return name;
+                             less_than_struct_with_name_and_id<ResourceTablePackageView, uint8_t>);
-  }
+        if (package_it == view.packages.end() || package_it->name != package_key.first ||
-  return {};
+            package_it->id != package_key.second) {
+          ResourceTablePackageView new_package{std::string(package_key.first), package_key.second};
+          package_it = view.packages.insert(package_it, new_package);
         }
 
-static StringPiece SkipNameValidator(const StringPiece& /*name*/) {
+        auto type_it = std::lower_bound(package_it->types.begin(), package_it->types.end(),
-  return {};
+                                        type_key, less_than_type_and_id<ResourceTableTypeView>);
+        if (type_it == package_it->types.end() || type_key.first != type_it->type ||
+            type_it->id != type_key.second) {
+          ResourceTableTypeView new_type{type_key.first, type_key.second};
+          type_it = package_it->types.insert(type_it, new_type);
         }
 
-bool ResourceTable::AddResource(const ResourceNameRef& name,
+        if (entry->visibility.level == Visibility::Level::kPublic) {
-                                const ConfigDescription& config,
+          // Only mark the type visibility level as public, it doesn't care about being private.
-                                const StringPiece& product,
+          type_it->visibility_level = Visibility::Level::kPublic;
-                                std::unique_ptr<Value> value,
-                                IDiagnostics* diag) {
-  return AddResourceImpl(name, ResourceId{}, config, product, std::move(value),
-                         (validate_resources_ ? ResourceNameValidator : SkipNameValidator),
-                         (validate_resources_ ? ResolveValueCollision : IgnoreCollision), diag);
         }
 
-bool ResourceTable::AddResourceWithId(const ResourceNameRef& name, const ResourceId& res_id,
+        auto entry_it =
-                                      const ConfigDescription& config, const StringPiece& product,
+            std::lower_bound(type_it->entries.begin(), type_it->entries.end(), entry_key,
-                                      std::unique_ptr<Value> value, IDiagnostics* diag) {
+                             less_than_struct_with_name_and_id_pointer<ResourceEntry, ResourceId>);
-  return AddResourceImpl(name, res_id, config, product, std::move(value),
+        type_it->entries.insert(entry_it, entry.get());
-                         (validate_resources_ ? ResourceNameValidator : SkipNameValidator),
+      }
-                         (validate_resources_ ? ResolveValueCollision : IgnoreCollision), diag);
+    }
   }
 
-bool ResourceTable::AddResourceMangled(const ResourceNameRef& name, const ConfigDescription& config,
+  return view;
-                                       const StringPiece& product, std::unique_ptr<Value> value,
-                                       IDiagnostics* diag) {
-  return AddResourceImpl(name, ResourceId{}, config, product, std::move(value), SkipNameValidator,
-                         (validate_resources_ ? ResolveValueCollision : IgnoreCollision), diag);
 }
 
-bool ResourceTable::AddResourceWithIdMangled(const ResourceNameRef& name, const ResourceId& id,
+bool ResourceTable::AddResource(NewResource&& res, IDiagnostics* diag) {
-                                             const ConfigDescription& config,
+  CHECK(diag != nullptr) << "Diagnostic pointer is null";
-                                             const StringPiece& product,
-                                             std::unique_ptr<Value> value, IDiagnostics* diag) {
-  return AddResourceImpl(name, id, config, product, std::move(value), SkipNameValidator,
-                         (validate_resources_ ? ResolveValueCollision : IgnoreCollision), diag);
-}
 
-bool ResourceTable::ValidateName(NameValidator name_validator, const ResourceNameRef& name,
+  const bool validate = validation_ == Validation::kEnabled;
-                                 const Source& source, IDiagnostics* diag) {
+  const Source source = res.value ? res.value->GetSource() : Source{};
-  const StringPiece bad_char = name_validator(name.entry);
+  if (validate && !res.allow_mangled && !IsValidResourceEntryName(res.name.entry)) {
-  if (!bad_char.empty()) {
-    diag->Error(DiagMessage(source) << "resource '" << name << "' has invalid entry name '"
-                                    << name.entry << "'. Invalid character '" << bad_char << "'");
-    return false;
-  }
-  return true;
-}
-
-bool ResourceTable::AddResourceImpl(const ResourceNameRef& name, const ResourceId& res_id,
-                                    const ConfigDescription& config, const StringPiece& product,
-                                    std::unique_ptr<Value> value, NameValidator name_validator,
-                                    const CollisionResolverFunc& conflict_resolver,
-                                    IDiagnostics* diag) {
-  CHECK(value != nullptr);
-  CHECK(diag != nullptr);
-
-  const Source& source = value->GetSource();
-  if (!ValidateName(name_validator, name, source, diag)) {
-    return false;
-  }
-
-  // Check for package names appearing twice with two different package ids
-  ResourceTablePackage* package = FindOrCreatePackage(name.package);
-  if (res_id.is_valid() && package->id && package->id.value() != res_id.package_id()) {
     diag->Error(DiagMessage(source)
-                    << "trying to add resource '" << name << "' with ID " << res_id
+                << "resource '" << res.name << "' has invalid entry name '" << res.name.entry);
-                    << " but package '" << package->name << "' already has ID "
-                    << StringPrintf("%02x", package->id.value()));
     return false;
   }
 
-  // Whether or not to error on duplicate resources
+  if (res.id.has_value() && !res.id->first.is_valid()) {
-  bool check_id = validate_resources_ && res_id.is_valid();
+    diag->Error(DiagMessage(source) << "trying to add resource '" << res.name << "' with ID "
-  // Whether or not to create a duplicate resource if the id does not match
+                                    << res.id->first << " but that ID is invalid");
-  bool use_id = !validate_resources_ && res_id.is_valid();
+    return false;
+  }
 
-  ResourceTableType* type = package->FindOrCreateType(name.type, use_id ? res_id.type_id()
+  auto package = FindOrCreatePackage(res.name.package);
-                                                                        : Maybe<uint8_t>());
+  auto type = package->FindOrCreateType(res.name.type);
+  auto entry_it = std::equal_range(type->entries.begin(), type->entries.end(), res.name.entry,
+                                   NameEqualRange<ResourceEntry>{});
+  const size_t entry_count = std::distance(entry_it.first, entry_it.second);
 
-  // Check for types appearing twice with two different type ids
+  ResourceEntry* entry;
-  if (check_id && type->id && type->id.value() != res_id.type_id()) {
+  if (entry_count == 0) {
+    // Adding a new resource
+    entry = type->CreateEntry(res.name.entry);
+  } else if (entry_count == 1) {
+    // Assume that the existing resource is being modified
+    entry = entry_it.first->get();
+  } else {
+    // Multiple resources with the same name exist in the resource table. The only way to
+    // distinguish between them is using resource id since each resource should have a unique id.
+    CHECK(res.id.has_value()) << "ambiguous modification of resource entry '" << res.name
+                              << "' without specifying a resource id.";
+    entry = entry_it.first->get();
+    for (auto it = entry_it.first; it != entry_it.second; ++it) {
+      CHECK((bool)(*it)->id) << "ambiguous modification of resource entry '" << res.name
+                             << "' with multiple entries without resource ids";
+      if ((*it)->id == res.id->first) {
+        entry = it->get();
+        break;
+      }
+    }
+  }
+
+  if (res.id.has_value()) {
+    if (entry->id && entry->id.value() != res.id->first) {
+      if (res.id->second != OnIdConflict::CREATE_ENTRY) {
         diag->Error(DiagMessage(source)
-                    << "trying to add resource '" << name << "' with ID " << res_id
+                    << "trying to add resource '" << res.name << "' with ID " << res.id->first
-                    << " but type '" << type->type << "' already has ID "
+                    << " but resource already has ID " << entry->id.value());
-                    << StringPrintf("%02x", type->id.value()));
         return false;
       }
-
+      entry = type->CreateEntry(res.name.entry);
-  ResourceEntry* entry = type->FindOrCreateEntry(name.entry, use_id ? res_id.entry_id()
+    }
-                                                                    : Maybe<uint16_t>());
+    entry->id = res.id->first;
-
-  // Check for entries appearing twice with two different entry ids
-  if (check_id && entry->id && entry->id.value() != res_id.entry_id()) {
-    diag->Error(DiagMessage(source)
-                    << "trying to add resource '" << name << "' with ID " << res_id
-                    << " but resource already has ID "
-                    << ResourceId(package->id.value(), type->id.value(), entry->id.value()));
-    return false;
   }
 
-  ResourceConfigValue* config_value = entry->FindOrCreateValue(config, product);
+  if (res.visibility.has_value()) {
+    // Only mark the type visibility level as public, it doesn't care about being private.
+    if (res.visibility->level == Visibility::Level::kPublic) {
+      type->visibility_level = Visibility::Level::kPublic;
+    }
+
+    if (res.visibility->level > entry->visibility.level) {
+      // This symbol definition takes precedence, replace.
+      entry->visibility = res.visibility.value();
+    }
+  }
+
+  if (res.overlayable.has_value()) {
+    if (entry->overlayable_item) {
+      diag->Error(DiagMessage(res.overlayable->source)
+                  << "duplicate overlayable declaration for resource '" << res.name << "'");
+      diag->Error(DiagMessage(entry->overlayable_item.value().source)
+                  << "previous declaration here");
+      return false;
+    }
+    entry->overlayable_item = res.overlayable.value();
+  }
+
+  if (res.allow_new.has_value()) {
+    entry->allow_new = res.allow_new.value();
+  }
+
+  if (res.value != nullptr) {
+    auto config_value = entry->FindOrCreateValue(res.config, res.product);
     if (!config_value->value) {
       // Resource does not exist, add it now.
-    config_value->value = std::move(value);
+      config_value->value = std::move(res.value);
     } else {
-    switch (conflict_resolver(config_value->value.get(), value.get())) {
+      // When validation is enabled, ensure that a resource cannot have multiple values defined for
+      // the same configuration.
+      auto result = validate ? ResolveValueCollision(config_value->value.get(), res.value.get())
+                             : CollisionResult::kKeepBoth;
+      switch (result) {
         case CollisionResult::kKeepBoth:
           // Insert the value ignoring for duplicate configurations
-        entry->values.push_back(util::make_unique<ResourceConfigValue>(config, product));
+          entry->values.push_back(util::make_unique<ResourceConfigValue>(res.config, res.product));
-        entry->values.back()->value = std::move(value);
+          entry->values.back()->value = std::move(res.value);
           break;
 
         case CollisionResult::kTakeNew:
           // Take the incoming value.
-        config_value->value = std::move(value);
+          config_value->value = std::move(res.value);
           break;
 
         case CollisionResult::kConflict:
-        diag->Error(DiagMessage(source) << "duplicate value for resource '" << name << "' "
+          diag->Error(DiagMessage(source) << "duplicate value for resource '" << res.name << "' "
-                                        << "with config '" << config << "'");
+                                          << "with config '" << res.config << "'");
           diag->Error(DiagMessage(source) << "resource previously defined here");
           return false;
 
@@ -462,167 +473,11 @@ bool ResourceTable::AddResourceImpl(const ResourceNameRef& name, const ResourceI
           break;
       }
     }
-
-  if (res_id.is_valid()) {
-    package->id = res_id.package_id();
-    type->id = res_id.type_id();
-    entry->id = res_id.entry_id();
   }
 
   return true;
 }
 
-bool ResourceTable::GetValidateResources() {
-  return validate_resources_;
-}
-
-bool ResourceTable::SetVisibility(const ResourceNameRef& name, const Visibility& visibility,
-                                  IDiagnostics* diag) {
-  return SetVisibilityImpl(name, visibility, {}, ResourceNameValidator, diag);
-}
-
-bool ResourceTable::SetVisibilityWithId(const ResourceNameRef& name, const Visibility& visibility,
-                                        const ResourceId& res_id, IDiagnostics* diag) {
-  return SetVisibilityImpl(name, visibility, res_id, ResourceNameValidator, diag);
-}
-
-bool ResourceTable::SetVisibilityWithIdMangled(const ResourceNameRef& name,
-                                               const Visibility& visibility,
-                                               const ResourceId& res_id, IDiagnostics* diag) {
-  return SetVisibilityImpl(name, visibility, res_id, SkipNameValidator, diag);
-}
-
-bool ResourceTable::SetVisibilityImpl(const ResourceNameRef& name, const Visibility& visibility,
-                                      const ResourceId& res_id, NameValidator name_validator,
-                                      IDiagnostics* diag) {
-  CHECK(diag != nullptr);
-
-  const Source& source = visibility.source;
-  if (!ValidateName(name_validator, name, source, diag)) {
-    return false;
-  }
-
-  // Check for package names appearing twice with two different package ids
-  ResourceTablePackage* package = FindOrCreatePackage(name.package);
-  if (res_id.is_valid() && package->id && package->id.value() != res_id.package_id()) {
-    diag->Error(DiagMessage(source)
-                    << "trying to add resource '" << name << "' with ID " << res_id
-                    << " but package '" << package->name << "' already has ID "
-                    << StringPrintf("%02x", package->id.value()));
-    return false;
-  }
-
-  // Whether or not to error on duplicate resources
-  bool check_id = validate_resources_ && res_id.is_valid();
-  // Whether or not to create a duplicate resource if the id does not match
-  bool use_id = !validate_resources_ && res_id.is_valid();
-
-  ResourceTableType* type = package->FindOrCreateType(name.type, use_id ? res_id.type_id()
-                                                                        : Maybe<uint8_t>());
-
-  // Check for types appearing twice with two different type ids
-  if (check_id && type->id && type->id.value() != res_id.type_id()) {
-    diag->Error(DiagMessage(source)
-                    << "trying to add resource '" << name << "' with ID " << res_id
-                    << " but type '" << type->type << "' already has ID "
-                    << StringPrintf("%02x", type->id.value()));
-    return false;
-  }
-
-  ResourceEntry* entry = type->FindOrCreateEntry(name.entry, use_id ? res_id.entry_id()
-                                                                    : Maybe<uint16_t>());
-
-  // Check for entries appearing twice with two different entry ids
-  if (check_id && entry->id && entry->id.value() != res_id.entry_id()) {
-    diag->Error(DiagMessage(source)
-                    << "trying to add resource '" << name << "' with ID " << res_id
-                    << " but resource already has ID "
-                    << ResourceId(package->id.value(), type->id.value(), entry->id.value()));
-    return false;
-  }
-
-  if (res_id.is_valid()) {
-    package->id = res_id.package_id();
-    type->id = res_id.type_id();
-    entry->id = res_id.entry_id();
-  }
-
-  // Only mark the type visibility level as public, it doesn't care about being private.
-  if (visibility.level == Visibility::Level::kPublic) {
-    type->visibility_level = Visibility::Level::kPublic;
-  }
-
-  if (visibility.level == Visibility::Level::kUndefined &&
-      entry->visibility.level != Visibility::Level::kUndefined) {
-    // We can't undefine a symbol (remove its visibility). Ignore.
-    return true;
-  }
-
-  if (visibility.level < entry->visibility.level) {
-    // We can't downgrade public to private. Ignore.
-    return true;
-  }
-
-  // This symbol definition takes precedence, replace.
-  entry->visibility = visibility;
-  return true;
-}
-
-bool ResourceTable::SetAllowNew(const ResourceNameRef& name, const AllowNew& allow_new,
-                                IDiagnostics* diag) {
-  return SetAllowNewImpl(name, allow_new, ResourceNameValidator, diag);
-}
-
-bool ResourceTable::SetAllowNewMangled(const ResourceNameRef& name, const AllowNew& allow_new,
-                                       IDiagnostics* diag) {
-  return SetAllowNewImpl(name, allow_new, SkipNameValidator, diag);
-}
-
-bool ResourceTable::SetAllowNewImpl(const ResourceNameRef& name, const AllowNew& allow_new,
-                                    NameValidator name_validator, IDiagnostics* diag) {
-  CHECK(diag != nullptr);
-
-  if (!ValidateName(name_validator, name, allow_new.source, diag)) {
-    return false;
-  }
-
-  ResourceTablePackage* package = FindOrCreatePackage(name.package);
-  ResourceTableType* type = package->FindOrCreateType(name.type);
-  ResourceEntry* entry = type->FindOrCreateEntry(name.entry);
-  entry->allow_new = allow_new;
-  return true;
-}
-
-bool ResourceTable::SetOverlayable(const ResourceNameRef& name, const OverlayableItem& overlayable,
-                                   IDiagnostics* diag) {
-  return SetOverlayableImpl(name, overlayable, ResourceNameValidator, diag);
-}
-
-bool ResourceTable::SetOverlayableImpl(const ResourceNameRef& name,
-                                       const OverlayableItem& overlayable,
-                                       NameValidator name_validator, IDiagnostics *diag) {
-  CHECK(diag != nullptr);
-
-  if (!ValidateName(name_validator, name, overlayable.source, diag)) {
-    return false;
-  }
-
-  ResourceTablePackage* package = FindOrCreatePackage(name.package);
-  ResourceTableType* type = package->FindOrCreateType(name.type);
-  ResourceEntry* entry = type->FindOrCreateEntry(name.entry);
-
-  if (entry->overlayable_item) {
-    diag->Error(DiagMessage(overlayable.source)
-                << "duplicate overlayable declaration for resource '" << name << "'");
-    diag->Error(DiagMessage(entry->overlayable_item.value().source)
-                << "previous declaration here");
-    return false;
-  }
-
-  entry->overlayable_item = overlayable;
-  return true;
-}
-
 Maybe<ResourceTable::SearchResult> ResourceTable::FindResource(const ResourceNameRef& name) const {
   ResourceTablePackage* package = FindPackage(name.package);
   if (package == nullptr) {
@@ -641,17 +496,38 @@ Maybe<ResourceTable::SearchResult> ResourceTable::FindResource(const ResourceNam
   return SearchResult{package, type, entry};
 }
 
+Maybe<ResourceTable::SearchResult> ResourceTable::FindResource(const ResourceNameRef& name,
+                                                               ResourceId id) const {
+  ResourceTablePackage* package = FindPackage(name.package);
+  if (package == nullptr) {
+    return {};
+  }
+
+  ResourceTableType* type = package->FindType(name.type);
+  if (type == nullptr) {
+    return {};
+  }
+
+  auto entry_it = std::equal_range(type->entries.begin(), type->entries.end(), name.entry,
+                                   NameEqualRange<ResourceEntry>{});
+  for (auto it = entry_it.first; it != entry_it.second; ++it) {
+    if ((*it)->id == id) {
+      return SearchResult{package, type, it->get()};
+    }
+  }
+  return {};
+}
+
 std::unique_ptr<ResourceTable> ResourceTable::Clone() const {
   std::unique_ptr<ResourceTable> new_table = util::make_unique<ResourceTable>();
   for (const auto& pkg : packages) {
-    ResourceTablePackage* new_pkg = new_table->CreatePackage(pkg->name, pkg->id);
+    ResourceTablePackage* new_pkg = new_table->FindOrCreatePackage(pkg->name);
     for (const auto& type : pkg->types) {
       ResourceTableType* new_type = new_pkg->FindOrCreateType(type->type);
-      new_type->id = type->id;
       new_type->visibility_level = type->visibility_level;
 
       for (const auto& entry : type->entries) {
-        ResourceEntry* new_entry = new_type->FindOrCreateEntry(entry->name);
+        ResourceEntry* new_entry = new_type->CreateEntry(entry->name);
         new_entry->id = entry->id;
         new_entry->visibility = entry->visibility;
         new_entry->allow_new = entry->allow_new;
@@ -668,4 +544,51 @@ std::unique_ptr<ResourceTable> ResourceTable::Clone() const {
   return new_table;
 }
 
+NewResourceBuilder::NewResourceBuilder(const ResourceNameRef& name) {
+  res_.name = name.ToResourceName();
+}
+
+NewResourceBuilder::NewResourceBuilder(const std::string& name) {
+  ResourceNameRef ref;
+  CHECK(ResourceUtils::ParseResourceName(name, &ref)) << "invalid resource name: " << name;
+  res_.name = ref.ToResourceName();
+}
+
+NewResourceBuilder& NewResourceBuilder::SetValue(std::unique_ptr<Value> value,
+                                                 android::ConfigDescription config,
+                                                 std::string product) {
+  res_.value = std::move(value);
+  res_.config = std::move(config);
+  res_.product = std::move(product);
+  return *this;
+}
+
+NewResourceBuilder& NewResourceBuilder::SetId(ResourceId id, OnIdConflict on_conflict) {
+  res_.id = std::make_pair(id, on_conflict);
+  return *this;
+}
+
+NewResourceBuilder& NewResourceBuilder::SetVisibility(Visibility visibility) {
+  res_.visibility = std::move(visibility);
+  return *this;
+}
+
+NewResourceBuilder& NewResourceBuilder::SetOverlayable(OverlayableItem overlayable) {
+  res_.overlayable = std::move(overlayable);
+  return *this;
+}
+NewResourceBuilder& NewResourceBuilder::SetAllowNew(AllowNew allow_new) {
+  res_.allow_new = std::move(allow_new);
+  return *this;
+}
+
+NewResourceBuilder& NewResourceBuilder::SetAllowMangled(bool allow_mangled) {
+  res_.allow_mangled = allow_mangled;
+  return *this;
+}
+
+NewResource NewResourceBuilder::Build() {
+  return std::move(res_);
+}
+
 }  // namespace aapt

tools/aapt2/ResourceTable.h

@@ -109,7 +109,7 @@ class ResourceEntry {
   const std::string name;
 
   // The entry ID for this resource (the EEEE in 0xPPTTEEEE).
-  Maybe<uint16_t> id;
+  Maybe<ResourceId> id;
 
   // Whether this resource is public (and must maintain the same entry ID across builds).
   Visibility visibility;
@@ -124,10 +124,10 @@ class ResourceEntry {
 
   explicit ResourceEntry(const android::StringPiece& name) : name(name.to_string()) {}
 
-  ResourceConfigValue* FindValue(const android::ConfigDescription& config);
-
   ResourceConfigValue* FindValue(const android::ConfigDescription& config,
-                                 const android::StringPiece& product);
+                                 android::StringPiece product = {});
+  const ResourceConfigValue* FindValue(const android::ConfigDescription& config,
+                                       android::StringPiece product = {}) const;
 
   ResourceConfigValue* FindOrCreateValue(const android::ConfigDescription& config,
                                          const android::StringPiece& product);
@@ -156,9 +156,6 @@ class ResourceTableType {
   // The logical type of resource (string, drawable, layout, etc.).
   const ResourceType type;
 
-  // The type ID for this resource (the TT in 0xPPTTEEEE).
-  Maybe<uint8_t> id;
-
   // Whether this type is public (and must maintain the same type ID across builds).
   Visibility::Level visibility_level = Visibility::Level::kUndefined;
 
@@ -167,10 +164,9 @@ class ResourceTableType {
 
   explicit ResourceTableType(const ResourceType type) : type(type) {}
 
-  ResourceEntry* FindEntry(const android::StringPiece& name,
+  ResourceEntry* CreateEntry(const android::StringPiece& name);
-                           Maybe<uint16_t> id = Maybe<uint16_t>());
+  ResourceEntry* FindEntry(const android::StringPiece& name) const;
-  ResourceEntry* FindOrCreateEntry(const android::StringPiece& name,
+  ResourceEntry* FindOrCreateEntry(const android::StringPiece& name);
-                                   Maybe<uint16_t> id = Maybe<uint16_t>());
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ResourceTableType);
@@ -180,70 +176,99 @@ class ResourceTablePackage {
  public:
   std::string name;
 
-  // The package ID (the PP in 0xPPTTEEEE).
-  Maybe<uint8_t> id;
-
   std::vector<std::unique_ptr<ResourceTableType>> types;
 
+  explicit ResourceTablePackage(const android::StringPiece& name) : name(name.to_string()) {
+  }
+
   ResourceTablePackage() = default;
-  ResourceTableType* FindType(ResourceType type, Maybe<uint8_t> id = Maybe<uint8_t>());
+  ResourceTableType* FindType(ResourceType type) const;
-  ResourceTableType* FindOrCreateType(const ResourceType type,
+  ResourceTableType* FindOrCreateType(ResourceType type);
-                                      Maybe<uint8_t> id = Maybe<uint8_t>());
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ResourceTablePackage);
 };
 
+struct ResourceTableTypeView {
+  ResourceType type;
+  Maybe<uint8_t> id;
+  Visibility::Level visibility_level = Visibility::Level::kUndefined;
+
+  // Entries sorted in ascending entry id order. If ids have not been assigned, the entries are
+  //  // sorted lexicographically.
+  std::vector<const ResourceEntry*> entries;
+};
+
+struct ResourceTablePackageView {
+  std::string name;
+  Maybe<uint8_t> id;
+  // Types sorted in ascending type id order. If ids have not been assigned, the types are sorted by
+  // their declaration order in the ResourceType enum.
+  std::vector<ResourceTableTypeView> types;
+};
+
+struct ResourceTableView {
+  // Packages sorted in ascending package id order. If ids have not been assigned, the packages are
+  // sorted lexicographically.
+  std::vector<ResourceTablePackageView> packages;
+};
+
+enum class OnIdConflict {
+  // If the resource entry already exists but has a different resource id, the resource value will
+  // not be added to the table.
+  ERROR,
+
+  // If the resource entry already exists but has a different resource id, create a new resource
+  // with this resource name and id combination.
+  CREATE_ENTRY,
+};
+
+struct NewResource {
+  ResourceName name;
+  std::unique_ptr<Value> value;
+  android::ConfigDescription config;
+  std::string product;
+  std::optional<std::pair<ResourceId, OnIdConflict>> id;
+  std::optional<Visibility> visibility;
+  std::optional<OverlayableItem> overlayable;
+  std::optional<AllowNew> allow_new;
+  bool allow_mangled = false;
+};
+
+struct NewResourceBuilder {
+  explicit NewResourceBuilder(const ResourceNameRef& name);
+  explicit NewResourceBuilder(const std::string& name);
+  NewResourceBuilder& SetValue(std::unique_ptr<Value> value, android::ConfigDescription config = {},
+                               std::string product = {});
+  NewResourceBuilder& SetId(ResourceId id, OnIdConflict on_conflict = OnIdConflict::ERROR);
+  NewResourceBuilder& SetVisibility(Visibility id);
+  NewResourceBuilder& SetOverlayable(OverlayableItem overlayable);
+  NewResourceBuilder& SetAllowNew(AllowNew allow_new);
+  NewResourceBuilder& SetAllowMangled(bool allow_mangled);
+  NewResource Build();
+
+ private:
+  NewResource res_;
+};
+
 // The container and index for all resources defined for an app.
 class ResourceTable {
  public:
-  ResourceTable() = default;
+  enum class Validation {
-  explicit ResourceTable(bool validate_resources) : validate_resources_(validate_resources) {}
+    kEnabled,
+    kDisabled,
+  };
 
   enum class CollisionResult { kKeepBoth, kKeepOriginal, kConflict, kTakeNew };
 
-  using CollisionResolverFunc = std::function<CollisionResult(Value*, Value*)>;
+  ResourceTable() = default;
+  explicit ResourceTable(Validation validation);
 
-  // When a collision of resources occurs, this method decides which value to keep.
+  bool AddResource(NewResource&& res, IDiagnostics* diag);
-  static CollisionResult ResolveValueCollision(Value* existing, Value* incoming);
 
-  // When a collision of resources occurs, this method keeps both values
+  // Retrieves a sorted a view of the packages, types, and entries sorted in ascending resource id
-  static CollisionResult IgnoreCollision(Value* existing, Value* incoming);
+  // order.
-
+  ResourceTableView GetPartitionedView() const;
-  bool AddResource(const ResourceNameRef& name, const android::ConfigDescription& config,
-                   const android::StringPiece& product, std::unique_ptr<Value> value,
-                   IDiagnostics* diag);
-
-  bool AddResourceWithId(const ResourceNameRef& name, const ResourceId& res_id,
-                         const android::ConfigDescription& config,
-                         const android::StringPiece& product, std::unique_ptr<Value> value,
-                         IDiagnostics* diag);
-
-  // Same as AddResource, but doesn't verify the validity of the name. This is used
-  // when loading resources from an existing binary resource table that may have mangled names.
-  bool AddResourceMangled(const ResourceNameRef& name, const android::ConfigDescription& config,
-                          const android::StringPiece& product, std::unique_ptr<Value> value,
-                          IDiagnostics* diag);
-
-  bool AddResourceWithIdMangled(const ResourceNameRef& name, const ResourceId& id,
-                                const android::ConfigDescription& config,
-                                const android::StringPiece& product, std::unique_ptr<Value> value,
-                                IDiagnostics* diag);
-
-  bool GetValidateResources();
-
-  bool SetVisibility(const ResourceNameRef& name, const Visibility& visibility, IDiagnostics* diag);
-  bool SetVisibilityWithId(const ResourceNameRef& name, const Visibility& visibility,
-                           const ResourceId& res_id, IDiagnostics* diag);
-  bool SetVisibilityWithIdMangled(const ResourceNameRef& name, const Visibility& visibility,
-                                  const ResourceId& res_id, IDiagnostics* diag);
-
-  bool SetOverlayable(const ResourceNameRef& name, const OverlayableItem& overlayable,
-                      IDiagnostics *diag);
-
-  bool SetAllowNew(const ResourceNameRef& name, const AllowNew& allow_new, IDiagnostics* diag);
-  bool SetAllowNewMangled(const ResourceNameRef& name, const AllowNew& allow_new,
-                          IDiagnostics* diag);
 
   struct SearchResult {
     ResourceTablePackage* package;
@@ -252,23 +277,19 @@ class ResourceTable {
   };
 
   Maybe<SearchResult> FindResource(const ResourceNameRef& name) const;
+  Maybe<SearchResult> FindResource(const ResourceNameRef& name, ResourceId id) const;
 
   // Returns the package struct with the given name, or nullptr if such a package does not
   // exist. The empty string is a valid package and typically is used to represent the
   // 'current' package before it is known to the ResourceTable.
   ResourceTablePackage* FindPackage(const android::StringPiece& name) const;
-
+  ResourceTablePackage* FindOrCreatePackage(const android::StringPiece& name);
-  ResourceTablePackage* FindPackageById(uint8_t id) const;
-
-  ResourceTablePackage* CreatePackage(const android::StringPiece& name, Maybe<uint8_t> id = {});
-
-  // Attempts to find a package having the specified name and ID. If not found, a new package
-  // of the specified parameters is created and returned.
-  ResourceTablePackage* CreatePackageAllowingDuplicateNames(const android::StringPiece& name,
-                                                            const Maybe<uint8_t> id);
 
   std::unique_ptr<ResourceTable> Clone() const;
 
+  // When a collision of resources occurs, this method decides which value to keep.
+  static CollisionResult ResolveValueCollision(Value* existing, Value* incoming);
+
   // The string pool used by this resource table. Values that reference strings must use
   // this pool to create their strings.
   // NOTE: `string_pool` must come before `packages` so that it is destroyed after.
@@ -286,36 +307,9 @@ class ResourceTable {
   std::map<size_t, std::string> included_packages_;
 
  private:
-  // The function type that validates a symbol name. Returns a non-empty StringPiece representing
-  // the offending character (which may be more than one byte in UTF-8). Returns an empty string
-  // if the name was valid.
-  using NameValidator = android::StringPiece(const android::StringPiece&);
-
-  ResourceTablePackage* FindOrCreatePackage(const android::StringPiece& name);
-
-  bool ValidateName(NameValidator validator, const ResourceNameRef& name, const Source& source,
-                    IDiagnostics* diag);
-
-  bool AddResourceImpl(const ResourceNameRef& name, const ResourceId& res_id,
-                       const android::ConfigDescription& config,
-                       const android::StringPiece& product, std::unique_ptr<Value> value,
-                       NameValidator name_validator, const CollisionResolverFunc& conflict_resolver,
-                       IDiagnostics* diag);
-
-  bool SetVisibilityImpl(const ResourceNameRef& name, const Visibility& visibility,
-                         const ResourceId& res_id, NameValidator name_validator,
-                         IDiagnostics* diag);
-
-  bool SetAllowNewImpl(const ResourceNameRef& name, const AllowNew& allow_new,
-                       NameValidator name_validator, IDiagnostics* diag);
-
-  bool SetOverlayableImpl(const ResourceNameRef &name, const OverlayableItem& overlayable,
-                          NameValidator name_validator, IDiagnostics *diag);
-
-  // Controls whether the table validates resource names and prevents duplicate resource names
-  bool validate_resources_ = true;
-
   DISALLOW_COPY_AND_ASSIGN(ResourceTable);
+
+  Validation validation_ = Validation::kEnabled;
 };
 
 }  // namespace aapt

tools/aapt2/ResourceTable_test.cpp

@@ -37,31 +37,32 @@ namespace aapt {
 TEST(ResourceTableTest, FailToAddResourceWithBadName) {
   ResourceTable table;
 
-  EXPECT_FALSE(table.AddResource(
+  EXPECT_FALSE(
-      test::ParseNameOrDie("android:id/hey,there"), ConfigDescription{}, "",
+      table.AddResource(NewResourceBuilder(test::ParseNameOrDie("android:id/hey,there")).Build(),
-      test::ValueBuilder<Id>().SetSource("test.xml", 21u).Build(),
                         test::GetDiagnostics()));
 
-  EXPECT_FALSE(table.AddResource(
+  EXPECT_FALSE(
-      test::ParseNameOrDie("android:id/hey:there"), ConfigDescription{}, "",
+      table.AddResource(NewResourceBuilder(test::ParseNameOrDie("android:id/hey:there")).Build(),
-      test::ValueBuilder<Id>().SetSource("test.xml", 21u).Build(),
                         test::GetDiagnostics()));
 }
 
 TEST(ResourceTableTest, AddResourceWithWeirdNameWhenAddingMangledResources) {
   ResourceTable table;
 
-  EXPECT_TRUE(table.AddResourceMangled(
+  EXPECT_TRUE(
-      test::ParseNameOrDie("android:id/heythere       "), ConfigDescription{}, "",
+      table.AddResource(NewResourceBuilder(test::ParseNameOrDie("android:id/heythere       "))
-      test::ValueBuilder<Id>().SetSource("test.xml", 21u).Build(), test::GetDiagnostics()));
+                            .SetAllowMangled(true)
+                            .Build(),
+                        test::GetDiagnostics()));
 }
 
 TEST(ResourceTableTest, AddOneResource) {
   ResourceTable table;
 
   EXPECT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:attr/id"), ConfigDescription{}, "",
+      NewResourceBuilder(test::ParseNameOrDie("android:attr/id"))
-      test::ValueBuilder<Id>().SetSource("test/path/file.xml", 23u).Build(),
+          .SetValue(test::ValueBuilder<Id>().SetSource("test/path/file.xml", 23u).Build())
+          .Build(),
       test::GetDiagnostics()));
 
   EXPECT_THAT(test::GetValue<Id>(&table, "android:attr/id"), NotNull());
@@ -70,30 +71,34 @@ TEST(ResourceTableTest, AddOneResource) {
 TEST(ResourceTableTest, AddMultipleResources) {
   ResourceTable table;
 
-  ConfigDescription config;
   ConfigDescription language_config;
   memcpy(language_config.language, "pl", sizeof(language_config.language));
 
   EXPECT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:attr/layout_width"), config, "",
+      NewResourceBuilder(test::ParseNameOrDie("android:attr/layout_width"))
-      test::ValueBuilder<Id>().SetSource("test/path/file.xml", 10u).Build(),
+          .SetValue(test::ValueBuilder<Id>().SetSource("test/path/file.xml", 10u).Build())
+          .Build(),
       test::GetDiagnostics()));
 
   EXPECT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:attr/id"), config, "",
+      NewResourceBuilder(test::ParseNameOrDie("android:attr/id"))
-      test::ValueBuilder<Id>().SetSource("test/path/file.xml", 12u).Build(),
+          .SetValue(test::ValueBuilder<Id>().SetSource("test/path/file.xml", 12u).Build())
+          .Build(),
       test::GetDiagnostics()));
 
   EXPECT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:string/ok"), config, "",
+      NewResourceBuilder(test::ParseNameOrDie("android:string/ok"))
-      test::ValueBuilder<Id>().SetSource("test/path/file.xml", 14u).Build(),
+          .SetValue(test::ValueBuilder<Id>().SetSource("test/path/file.xml", 14u).Build())
+          .Build(),
       test::GetDiagnostics()));
 
-  EXPECT_TRUE(table.AddResource(
+  EXPECT_TRUE(
-      test::ParseNameOrDie("android:string/ok"), language_config, "",
+      table.AddResource(NewResourceBuilder(test::ParseNameOrDie("android:string/ok"))
-      test::ValueBuilder<BinaryPrimitive>(android::Res_value{})
+                            .SetValue(test::ValueBuilder<BinaryPrimitive>(android::Res_value{})
                                           .SetSource("test/path/file.xml", 20u)
                                           .Build(),
+                                      language_config)
+                            .Build(),
                         test::GetDiagnostics()));
 
   EXPECT_THAT(test::GetValue<Id>(&table, "android:attr/layout_width"), NotNull());
@@ -104,17 +109,19 @@ TEST(ResourceTableTest, AddMultipleResources) {
 
 TEST(ResourceTableTest, OverrideWeakResourceValue) {
   ResourceTable table;
-
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(test::ParseNameOrDie("android:attr/foo"))
-  ASSERT_TRUE(table.AddResource(test::ParseNameOrDie("android:attr/foo"), ConfigDescription{}, "",
+                                    .SetValue(test::AttributeBuilder().SetWeak(true).Build())
-                                test::AttributeBuilder().SetWeak(true).Build(),
+                                    .Build(),
                                 test::GetDiagnostics()));
 
   Attribute* attr = test::GetValue<Attribute>(&table, "android:attr/foo");
   ASSERT_THAT(attr, NotNull());
   EXPECT_TRUE(attr->IsWeak());
 
-  ASSERT_TRUE(table.AddResource(test::ParseNameOrDie("android:attr/foo"), ConfigDescription{}, "",
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(test::ParseNameOrDie("android:attr/foo"))
-                                util::make_unique<Attribute>(), test::GetDiagnostics()));
+                                    .SetValue(util::make_unique<Attribute>())
+                                    .Build(),
+                                test::GetDiagnostics()));
 
   attr = test::GetValue<Attribute>(&table, "android:attr/foo");
   ASSERT_THAT(attr, NotNull());
@@ -130,22 +137,26 @@ TEST(ResourceTableTest, AllowCompatibleDuplicateAttributes) {
   Attribute attr_two(android::ResTable_map::TYPE_STRING | android::ResTable_map::TYPE_REFERENCE);
   attr_two.SetWeak(true);
 
-  ASSERT_TRUE(table.AddResource(name, ConfigDescription{}, "",
+  ASSERT_TRUE(table.AddResource(
-                                util::make_unique<Attribute>(attr_one), test::GetDiagnostics()));
+      NewResourceBuilder(name).SetValue(util::make_unique<Attribute>(attr_one)).Build(),
-  ASSERT_TRUE(table.AddResource(name, ConfigDescription{}, "",
+      test::GetDiagnostics()));
-                                util::make_unique<Attribute>(attr_two), test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(
+      NewResourceBuilder(name).SetValue(util::make_unique<Attribute>(attr_two)).Build(),
+      test::GetDiagnostics()));
 }
 
 TEST(ResourceTableTest, ProductVaryingValues) {
   ResourceTable table;
-
+  ASSERT_TRUE(table.AddResource(
-  EXPECT_TRUE(table.AddResource(test::ParseNameOrDie("android:string/foo"),
+      NewResourceBuilder(test::ParseNameOrDie("android:string/foo"))
-                                test::ParseConfigOrDie("land"), "tablet",
+          .SetValue(util::make_unique<Id>(), test::ParseConfigOrDie("land"), "tablet")
-                                util::make_unique<Id>(),
+          .Build(),
       test::GetDiagnostics()));
-  EXPECT_TRUE(table.AddResource(test::ParseNameOrDie("android:string/foo"),
+
-                                test::ParseConfigOrDie("land"), "phone",
+  ASSERT_TRUE(table.AddResource(
-                                util::make_unique<Id>(),
+      NewResourceBuilder(test::ParseNameOrDie("android:string/foo"))
+          .SetValue(util::make_unique<Id>(), test::ParseConfigOrDie("land"), "phone")
+          .Build(),
       test::GetDiagnostics()));
 
   EXPECT_THAT(test::GetValueForConfigAndProduct<Id>(&table, "android:string/foo",test::ParseConfigOrDie("land"), "tablet"), NotNull());
@@ -203,22 +214,26 @@ TEST(ResourceTableTest, SetVisibility) {
   Visibility visibility;
   visibility.level = Visibility::Level::kPrivate;
   visibility.comment = "private";
-  ASSERT_TRUE(table.SetVisibility(name, visibility, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(name).SetVisibility(visibility).Build(),
+                                test::GetDiagnostics()));
   ASSERT_TRUE(VisibilityOfResource(table, name, Level::kPrivate, "private"));
 
   visibility.level = Visibility::Level::kUndefined;
   visibility.comment = "undefined";
-  ASSERT_TRUE(table.SetVisibility(name, visibility, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(name).SetVisibility(visibility).Build(),
+                                test::GetDiagnostics()));
   ASSERT_TRUE(VisibilityOfResource(table, name, Level::kPrivate, "private"));
 
   visibility.level = Visibility::Level::kPublic;
   visibility.comment = "public";
-  ASSERT_TRUE(table.SetVisibility(name, visibility, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(name).SetVisibility(visibility).Build(),
+                                test::GetDiagnostics()));
   ASSERT_TRUE(VisibilityOfResource(table, name, Level::kPublic, "public"));
 
   visibility.level = Visibility::Level::kPrivate;
   visibility.comment = "private";
-  ASSERT_TRUE(table.SetVisibility(name, visibility, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(name).SetVisibility(visibility).Build(),
+                                test::GetDiagnostics()));
   ASSERT_TRUE(VisibilityOfResource(table, name, Level::kPublic, "public"));
 }
 
@@ -230,14 +245,16 @@ TEST(ResourceTableTest, SetAllowNew) {
   Maybe<ResourceTable::SearchResult> result;
 
   allow_new.comment = "first";
-  ASSERT_TRUE(table.SetAllowNew(name, allow_new, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(name).SetAllowNew(allow_new).Build(),
+                                test::GetDiagnostics()));
   result = table.FindResource(name);
   ASSERT_TRUE(result);
   ASSERT_TRUE(result.value().entry->allow_new);
   ASSERT_THAT(result.value().entry->allow_new.value().comment, StrEq("first"));
 
   allow_new.comment = "second";
-  ASSERT_TRUE(table.SetAllowNew(name, allow_new, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(name).SetAllowNew(allow_new).Build(),
+                                test::GetDiagnostics()));
   result = table.FindResource(name);
   ASSERT_TRUE(result);
   ASSERT_TRUE(result.value().entry->allow_new);
@@ -255,7 +272,8 @@ TEST(ResourceTableTest, SetOverlayable) {
   overlayable_item.source = Source("res/values/overlayable.xml", 42);
 
   const ResourceName name = test::ParseNameOrDie("android:string/foo");
-  ASSERT_TRUE(table.SetOverlayable(name, overlayable_item, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(name).SetOverlayable(overlayable_item).Build(),
+                                test::GetDiagnostics()));
   Maybe<ResourceTable::SearchResult> search_result = table.FindResource(name);
 
   ASSERT_TRUE(search_result);
@@ -280,17 +298,20 @@ TEST(ResourceTableTest, SetMultipleOverlayableResources) {
   auto group = std::make_shared<Overlayable>("Name", "overlay://theme");
   OverlayableItem overlayable(group);
   overlayable.policies = PolicyFlags::PRODUCT_PARTITION;
-  ASSERT_TRUE(table.SetOverlayable(foo, overlayable, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(foo).SetOverlayable(overlayable).Build(),
+                                test::GetDiagnostics()));
 
   const ResourceName bar = test::ParseNameOrDie("android:string/bar");
   OverlayableItem overlayable2(group);
   overlayable2.policies = PolicyFlags::PRODUCT_PARTITION;
-  ASSERT_TRUE(table.SetOverlayable(bar, overlayable2, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(bar).SetOverlayable(overlayable2).Build(),
+                                test::GetDiagnostics()));
 
   const ResourceName baz = test::ParseNameOrDie("android:string/baz");
   OverlayableItem overlayable3(group);
   overlayable3.policies = PolicyFlags::VENDOR_PARTITION;
-  ASSERT_TRUE(table.SetOverlayable(baz, overlayable3, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(baz).SetOverlayable(overlayable3).Build(),
+                                test::GetDiagnostics()));
 }
 
 TEST(ResourceTableTest, SetOverlayableDifferentResourcesDifferentName) {
@@ -299,12 +320,14 @@ TEST(ResourceTableTest, SetOverlayableDifferentResourcesDifferentName) {
   const ResourceName foo = test::ParseNameOrDie("android:string/foo");
   OverlayableItem overlayable_item(std::make_shared<Overlayable>("Name", "overlay://theme"));
   overlayable_item.policies = PolicyFlags::PRODUCT_PARTITION;
-  ASSERT_TRUE(table.SetOverlayable(foo, overlayable_item, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(foo).SetOverlayable(overlayable_item).Build(),
+                                test::GetDiagnostics()));
 
   const ResourceName bar = test::ParseNameOrDie("android:string/bar");
   OverlayableItem overlayable_item2(std::make_shared<Overlayable>("Name2",  "overlay://theme"));
   overlayable_item2.policies = PolicyFlags::PRODUCT_PARTITION;
-  ASSERT_TRUE(table.SetOverlayable(bar, overlayable_item2, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(bar).SetOverlayable(overlayable_item2).Build(),
+                                test::GetDiagnostics()));
 }
 
 TEST(ResourceTableTest, SetOverlayableSameResourcesFail) {
@@ -313,10 +336,12 @@ TEST(ResourceTableTest, SetOverlayableSameResourcesFail) {
 
   auto overlayable = std::make_shared<Overlayable>("Name", "overlay://theme");
   OverlayableItem overlayable_item(overlayable);
-  ASSERT_TRUE(table.SetOverlayable(name, overlayable_item, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(name).SetOverlayable(overlayable_item).Build(),
+                                test::GetDiagnostics()));
 
   OverlayableItem overlayable_item2(overlayable);
-  ASSERT_FALSE(table.SetOverlayable(name, overlayable_item2, test::GetDiagnostics()));
+  ASSERT_FALSE(table.AddResource(NewResourceBuilder(name).SetOverlayable(overlayable_item2).Build(),
+                                 test::GetDiagnostics()));
 }
 
 TEST(ResourceTableTest,  SetOverlayableSameResourcesDifferentNameFail) {
@@ -325,51 +350,38 @@ TEST(ResourceTableTest,  SetOverlayableSameResourcesDifferentNameFail) {
 
   auto overlayable = std::make_shared<Overlayable>("Name", "overlay://theme");
   OverlayableItem overlayable_item(overlayable);
-  ASSERT_TRUE(table.SetOverlayable(name, overlayable_item, test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(name).SetOverlayable(overlayable_item).Build(),
+                                test::GetDiagnostics()));
 
   auto overlayable2 = std::make_shared<Overlayable>("Other", "overlay://theme");
   OverlayableItem overlayable_item2(overlayable2);
-  ASSERT_FALSE(table.SetOverlayable(name, overlayable_item2, test::GetDiagnostics()));
+  ASSERT_FALSE(table.AddResource(NewResourceBuilder(name).SetOverlayable(overlayable_item2).Build(),
+                                 test::GetDiagnostics()));
 }
 
-TEST(ResourceTableTest, AllowDuplictaeResourcesNames) {
+TEST(ResourceTableTest, ConflictingIds) {
-  ResourceTable table(/* validate_resources */ false);
+  ResourceTable table;
-
+  const ResourceName name = test::ParseNameOrDie("android:string/foo");
-  const ResourceName foo_name = test::ParseNameOrDie("android:bool/foo");
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder(name).SetId(0x01010000).Build(),
-  ASSERT_TRUE(table.AddResourceWithId(foo_name, ResourceId(0x7f0100ff), ConfigDescription{} , "",
-                                      test::BuildPrimitive(android::Res_value::TYPE_INT_BOOLEAN, 0),
                                 test::GetDiagnostics()));
-  ASSERT_TRUE(table.AddResourceWithId(foo_name, ResourceId(0x7f010100), ConfigDescription{} , "",
+  ASSERT_FALSE(table.AddResource(NewResourceBuilder(name).SetId(0x01010001).Build(),
-                                      test::BuildPrimitive(android::Res_value::TYPE_INT_BOOLEAN, 1),
+                                 test::GetDiagnostics()));
+}
+
+TEST(ResourceTableTest, ConflictingIdsCreateEntry) {
+  ResourceTable table;
+  const ResourceName name = test::ParseNameOrDie("android:string/foo");
+  ASSERT_TRUE(table.AddResource(
+      NewResourceBuilder(name).SetId(0x01010000, OnIdConflict::CREATE_ENTRY).Build(),
+      test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(
+      NewResourceBuilder(name).SetId(0x01010001, OnIdConflict::CREATE_ENTRY).Build(),
       test::GetDiagnostics()));
 
-  ASSERT_TRUE(table.SetVisibilityWithId(foo_name, Visibility{Visibility::Level::kPublic},
+  // Non-ambiguous query
-                                        ResourceId(0x7f0100ff), test::GetDiagnostics()));
+  ASSERT_TRUE(table.AddResource(
-  ASSERT_TRUE(table.SetVisibilityWithId(foo_name, Visibility{Visibility::Level::kPrivate},
+      NewResourceBuilder(name).SetId(0x01010001).SetValue(std::make_unique<Id>()).Build(),
-                                        ResourceId(0x7f010100), test::GetDiagnostics()));
+      test::GetDiagnostics()));
-
-  auto package = table.FindPackageById(0x7f);
-  ASSERT_THAT(package, NotNull());
-  auto type = package->FindType(ResourceType::kBool);
-  ASSERT_THAT(type, NotNull());
-
-  auto entry1 = type->FindEntry("foo", 0x00ff);
-  ASSERT_THAT(entry1, NotNull());
-  ASSERT_THAT(entry1->id, Eq(0x00ff));
-  ASSERT_THAT(entry1->values[0], NotNull());
-  ASSERT_THAT(entry1->values[0]->value, NotNull());
-  ASSERT_THAT(ValueCast<BinaryPrimitive>(entry1->values[0]->value.get()), NotNull());
-  ASSERT_THAT(ValueCast<BinaryPrimitive>(entry1->values[0]->value.get())->value.data, Eq(0u));
-  ASSERT_THAT(entry1->visibility.level, Visibility::Level::kPublic);
-
-  auto entry2 = type->FindEntry("foo", 0x0100);
-  ASSERT_THAT(entry2, NotNull());
-  ASSERT_THAT(entry2->id, Eq(0x0100));
-  ASSERT_THAT(entry2->values[0], NotNull());
-  ASSERT_THAT(entry1->values[0]->value, NotNull());
-  ASSERT_THAT(ValueCast<BinaryPrimitive>(entry2->values[0]->value.get()), NotNull());
-  ASSERT_THAT(ValueCast<BinaryPrimitive>(entry2->values[0]->value.get())->value.data, Eq(1u));
-  ASSERT_THAT(entry2->visibility.level, Visibility::Level::kPrivate);
 }
 
 }  // namespace aapt

tools/aapt2/cmd/Compile.cpp

@@ -190,17 +190,6 @@ static bool CompileTable(IAaptContext* context, const CompileOptions& options,
     }
   }
 
-  // Ensure we have the compilation package at least.
-  table.CreatePackage(context->GetCompilationPackage());
-
-  // Assign an ID to any package that has resources.
-  for (auto& pkg : table.packages) {
-    if (!pkg->id) {
-      // If no package ID was set while parsing (public identifiers), auto assign an ID.
-      pkg->id = context->GetPackageId();
-    }
-  }
-
   // Create the file/zip entry.
   if (!writer->StartEntry(output_path, 0)) {
     context->GetDiagnostics()->Error(DiagMessage(output_path) << "failed to open");

tools/aapt2/cmd/Compile_test.cpp

@@ -217,6 +217,7 @@ static void AssertTranslations(CommandTestFixture *ctf, std::string file_name,
   }, compiled_files_dir, &diag));
 
   std::unique_ptr<LoadedApk> apk = LoadedApk::LoadApkFromPath(out_apk, &diag);
+  ASSERT_NE(apk, nullptr);
 
   ResourceTable* table = apk->GetResourceTable();
   ASSERT_NE(table, nullptr);

tools/aapt2/cmd/Diff.cpp

@@ -95,17 +95,17 @@ static bool IsIdDiff(const Visibility::Level& level_a, const Maybe<Id>& id_a,
   return false;
 }
 
-static bool EmitResourceConfigValueDiff(IAaptContext* context, LoadedApk* apk_a,
+static bool EmitResourceConfigValueDiff(
-                                        ResourceTablePackage* pkg_a, ResourceTableType* type_a,
+    IAaptContext* context, LoadedApk* apk_a, const ResourceTablePackageView& pkg_a,
-                                        ResourceEntry* entry_a, ResourceConfigValue* config_value_a,
+    const ResourceTableTypeView& type_a, const ResourceEntry* entry_a,
-                                        LoadedApk* apk_b, ResourceTablePackage* pkg_b,
+    const ResourceConfigValue* config_value_a, LoadedApk* apk_b,
-                                        ResourceTableType* type_b, ResourceEntry* entry_b,
+    const ResourceTablePackageView& pkg_b, const ResourceTableTypeView& type_b,
-                                        ResourceConfigValue* config_value_b) {
+    const ResourceEntry* entry_b, const ResourceConfigValue* config_value_b) {
   Value* value_a = config_value_a->value.get();
   Value* value_b = config_value_b->value.get();
   if (!value_a->Equals(value_b)) {
     std::stringstream str_stream;
-    str_stream << "value " << pkg_a->name << ":" << type_a->type << "/" << entry_a->name
+    str_stream << "value " << pkg_a.name << ":" << type_a.type << "/" << entry_a->name
                << " config=" << config_value_a->config << " does not match:\n";
     value_a->Print(&str_stream);
     str_stream << "\n vs \n";
@@ -117,16 +117,17 @@ static bool EmitResourceConfigValueDiff(IAaptContext* context, LoadedApk* apk_a,
 }
 
 static bool EmitResourceEntryDiff(IAaptContext* context, LoadedApk* apk_a,
-                                  ResourceTablePackage* pkg_a, ResourceTableType* type_a,
+                                  const ResourceTablePackageView& pkg_a,
-                                  ResourceEntry* entry_a, LoadedApk* apk_b,
+                                  const ResourceTableTypeView& type_a, const ResourceEntry* entry_a,
-                                  ResourceTablePackage* pkg_b, ResourceTableType* type_b,
+                                  LoadedApk* apk_b, const ResourceTablePackageView& pkg_b,
-                                  ResourceEntry* entry_b) {
+                                  const ResourceTableTypeView& type_b,
+                                  const ResourceEntry* entry_b) {
   bool diff = false;
-  for (std::unique_ptr<ResourceConfigValue>& config_value_a : entry_a->values) {
+  for (const std::unique_ptr<ResourceConfigValue>& config_value_a : entry_a->values) {
-    ResourceConfigValue* config_value_b = entry_b->FindValue(config_value_a->config);
+    auto config_value_b = entry_b->FindValue(config_value_a->config);
     if (!config_value_b) {
       std::stringstream str_stream;
-      str_stream << "missing " << pkg_a->name << ":" << type_a->type << "/" << entry_a->name
+      str_stream << "missing " << pkg_a.name << ":" << type_a.type << "/" << entry_a->name
                  << " config=" << config_value_a->config;
       EmitDiffLine(apk_b->GetSource(), str_stream.str());
       diff = true;
@@ -138,35 +139,47 @@ static bool EmitResourceEntryDiff(IAaptContext* context, LoadedApk* apk_a,
   }
 
   // Check for any newly added config values.
-  for (std::unique_ptr<ResourceConfigValue>& config_value_b : entry_b->values) {
+  for (const std::unique_ptr<ResourceConfigValue>& config_value_b : entry_b->values) {
-    ResourceConfigValue* config_value_a = entry_a->FindValue(config_value_b->config);
+    auto config_value_a = entry_a->FindValue(config_value_b->config);
     if (!config_value_a) {
       std::stringstream str_stream;
-      str_stream << "new config " << pkg_b->name << ":" << type_b->type << "/" << entry_b->name
+      str_stream << "new config " << pkg_b.name << ":" << type_b.type << "/" << entry_b->name
                  << " config=" << config_value_b->config;
       EmitDiffLine(apk_b->GetSource(), str_stream.str());
       diff = true;
     }
   }
-  return false;
+  return diff;
 }
 
 static bool EmitResourceTypeDiff(IAaptContext* context, LoadedApk* apk_a,
-                                 ResourceTablePackage* pkg_a, ResourceTableType* type_a,
+                                 const ResourceTablePackageView& pkg_a,
-                                 LoadedApk* apk_b, ResourceTablePackage* pkg_b,
+                                 const ResourceTableTypeView& type_a, LoadedApk* apk_b,
-                                 ResourceTableType* type_b) {
+                                 const ResourceTablePackageView& pkg_b,
+                                 const ResourceTableTypeView& type_b) {
   bool diff = false;
-  for (std::unique_ptr<ResourceEntry>& entry_a : type_a->entries) {
+  auto entry_a_iter = type_a.entries.begin();
-    ResourceEntry* entry_b = type_b->FindEntry(entry_a->name);
+  auto entry_b_iter = type_b.entries.begin();
-    if (!entry_b) {
+  while (entry_a_iter != type_a.entries.end() || entry_b_iter != type_b.entries.end()) {
+    if (entry_b_iter == type_b.entries.end()) {
+      // Type A contains a type that type B does not have.
       std::stringstream str_stream;
-      str_stream << "missing " << pkg_a->name << ":" << type_a->type << "/" << entry_a->name;
+      str_stream << "missing " << pkg_a.name << ":" << type_a.type << "/" << (*entry_a_iter)->name;
+      EmitDiffLine(apk_a->GetSource(), str_stream.str());
+      diff = true;
+    } else if (entry_a_iter == type_a.entries.end()) {
+      // Type B contains a type that type A does not have.
+      std::stringstream str_stream;
+      str_stream << "new entry " << pkg_b.name << ":" << type_b.type << "/"
+                 << (*entry_b_iter)->name;
       EmitDiffLine(apk_b->GetSource(), str_stream.str());
       diff = true;
     } else {
+      const auto& entry_a = *entry_a_iter;
+      const auto& entry_b = *entry_a_iter;
       if (IsSymbolVisibilityDifferent(entry_a->visibility, entry_b->visibility)) {
         std::stringstream str_stream;
-        str_stream << pkg_a->name << ":" << type_a->type << "/" << entry_a->name
+        str_stream << pkg_a.name << ":" << type_a.type << "/" << entry_a->name
                    << " has different visibility (";
         if (entry_b->visibility.level == Visibility::Level::kPublic) {
           str_stream << "PUBLIC";
@@ -185,7 +198,7 @@ static bool EmitResourceTypeDiff(IAaptContext* context, LoadedApk* apk_a,
       } else if (IsIdDiff(entry_a->visibility.level, entry_a->id, entry_b->visibility.level,
                           entry_b->id)) {
         std::stringstream str_stream;
-        str_stream << pkg_a->name << ":" << type_a->type << "/" << entry_a->name
+        str_stream << pkg_a.name << ":" << type_a.type << "/" << entry_a->name
                    << " has different public ID (";
         if (entry_b->id) {
           str_stream << "0x" << std::hex << entry_b->id.value();
@@ -202,46 +215,43 @@ static bool EmitResourceTypeDiff(IAaptContext* context, LoadedApk* apk_a,
         EmitDiffLine(apk_b->GetSource(), str_stream.str());
         diff = true;
       }
-      diff |= EmitResourceEntryDiff(context, apk_a, pkg_a, type_a, entry_a.get(), apk_b, pkg_b,
+      diff |= EmitResourceEntryDiff(context, apk_a, pkg_a, type_a, entry_a, apk_b, pkg_b, type_b,
-                                    type_b, entry_b);
+                                    entry_b);
-    }
-  }
-
-  // Check for any newly added entries.
-  for (std::unique_ptr<ResourceEntry>& entry_b : type_b->entries) {
-    ResourceEntry* entry_a = type_a->FindEntry(entry_b->name);
-    if (!entry_a) {
-      std::stringstream str_stream;
-      str_stream << "new entry " << pkg_b->name << ":" << type_b->type << "/" << entry_b->name;
-      EmitDiffLine(apk_b->GetSource(), str_stream.str());
-      diff = true;
     }
   }
   return diff;
 }
 
 static bool EmitResourcePackageDiff(IAaptContext* context, LoadedApk* apk_a,
-                                    ResourceTablePackage* pkg_a, LoadedApk* apk_b,
+                                    const ResourceTablePackageView& pkg_a, LoadedApk* apk_b,
-                                    ResourceTablePackage* pkg_b) {
+                                    const ResourceTablePackageView& pkg_b) {
   bool diff = false;
-  for (std::unique_ptr<ResourceTableType>& type_a : pkg_a->types) {
+  auto type_a_iter = pkg_a.types.begin();
-    ResourceTableType* type_b = pkg_b->FindType(type_a->type);
+  auto type_b_iter = pkg_b.types.begin();
-    if (!type_b) {
+  while (type_a_iter != pkg_a.types.end() || type_b_iter != pkg_b.types.end()) {
+    if (type_b_iter == pkg_b.types.end()) {
+      // Type A contains a type that type B does not have.
       std::stringstream str_stream;
-      str_stream << "missing " << pkg_a->name << ":" << type_a->type;
+      str_stream << "missing " << pkg_a.name << ":" << type_a_iter->type;
       EmitDiffLine(apk_a->GetSource(), str_stream.str());
       diff = true;
-    } else {
+    } else if (type_a_iter == pkg_a.types.end()) {
-      if (type_a->visibility_level != type_b->visibility_level) {
+      // Type B contains a type that type A does not have.
       std::stringstream str_stream;
-        str_stream << pkg_a->name << ":" << type_a->type << " has different visibility (";
+      str_stream << "new type " << pkg_b.name << ":" << type_b_iter->type;
-        if (type_b->visibility_level == Visibility::Level::kPublic) {
+      EmitDiffLine(apk_b->GetSource(), str_stream.str());
+      diff = true;
+    } else {
+      if (type_a_iter->visibility_level != type_b_iter->visibility_level) {
+        std::stringstream str_stream;
+        str_stream << pkg_a.name << ":" << type_a_iter->type << " has different visibility (";
+        if (type_b_iter->visibility_level == Visibility::Level::kPublic) {
           str_stream << "PUBLIC";
         } else {
           str_stream << "PRIVATE";
         }
         str_stream << " vs ";
-        if (type_a->visibility_level == Visibility::Level::kPublic) {
+        if (type_a_iter->visibility_level == Visibility::Level::kPublic) {
           str_stream << "PUBLIC";
         } else {
           str_stream << "PRIVATE";
@@ -249,18 +259,18 @@ static bool EmitResourcePackageDiff(IAaptContext* context, LoadedApk* apk_a,
         str_stream << ")";
         EmitDiffLine(apk_b->GetSource(), str_stream.str());
         diff = true;
-      } else if (IsIdDiff(type_a->visibility_level, type_a->id, type_b->visibility_level,
+      } else if (IsIdDiff(type_a_iter->visibility_level, type_a_iter->id,
-                          type_b->id)) {
+                          type_b_iter->visibility_level, type_b_iter->id)) {
         std::stringstream str_stream;
-        str_stream << pkg_a->name << ":" << type_a->type << " has different public ID (";
+        str_stream << pkg_a.name << ":" << type_a_iter->type << " has different public ID (";
-        if (type_b->id) {
+        if (type_b_iter->id) {
-          str_stream << "0x" << std::hex << type_b->id.value();
+          str_stream << "0x" << std::hex << type_b_iter->id.value();
         } else {
           str_stream << "none";
         }
         str_stream << " vs ";
-        if (type_a->id) {
+        if (type_a_iter->id) {
-          str_stream << "0x " << std::hex << type_a->id.value();
+          str_stream << "0x " << std::hex << type_a_iter->id.value();
         } else {
           str_stream << "none";
         }
@@ -268,47 +278,44 @@ static bool EmitResourcePackageDiff(IAaptContext* context, LoadedApk* apk_a,
         EmitDiffLine(apk_b->GetSource(), str_stream.str());
         diff = true;
       }
-      diff |= EmitResourceTypeDiff(context, apk_a, pkg_a, type_a.get(), apk_b, pkg_b, type_b);
+      diff |= EmitResourceTypeDiff(context, apk_a, pkg_a, *type_a_iter, apk_b, pkg_b, *type_b_iter);
-    }
-  }
-
-  // Check for any newly added types.
-  for (std::unique_ptr<ResourceTableType>& type_b : pkg_b->types) {
-    ResourceTableType* type_a = pkg_a->FindType(type_b->type);
-    if (!type_a) {
-      std::stringstream str_stream;
-      str_stream << "new type " << pkg_b->name << ":" << type_b->type;
-      EmitDiffLine(apk_b->GetSource(), str_stream.str());
-      diff = true;
     }
   }
   return diff;
 }
 
 static bool EmitResourceTableDiff(IAaptContext* context, LoadedApk* apk_a, LoadedApk* apk_b) {
-  ResourceTable* table_a = apk_a->GetResourceTable();
+  const auto table_a = apk_a->GetResourceTable()->GetPartitionedView();
-  ResourceTable* table_b = apk_b->GetResourceTable();
+  const auto table_b = apk_b->GetResourceTable()->GetPartitionedView();
 
   bool diff = false;
-  for (std::unique_ptr<ResourceTablePackage>& pkg_a : table_a->packages) {
+  auto package_a_iter = table_a.packages.begin();
-    ResourceTablePackage* pkg_b = table_b->FindPackage(pkg_a->name);
+  auto package_b_iter = table_b.packages.begin();
-    if (!pkg_b) {
+  while (package_a_iter != table_a.packages.end() || package_b_iter != table_b.packages.end()) {
+    if (package_b_iter == table_b.packages.end()) {
+      // Table A contains a package that table B does not have.
       std::stringstream str_stream;
-      str_stream << "missing package " << pkg_a->name;
+      str_stream << "missing package " << package_a_iter->name;
+      EmitDiffLine(apk_a->GetSource(), str_stream.str());
+      diff = true;
+    } else if (package_a_iter == table_a.packages.end()) {
+      // Table B contains a package that table A does not have.
+      std::stringstream str_stream;
+      str_stream << "new package " << package_b_iter->name;
       EmitDiffLine(apk_b->GetSource(), str_stream.str());
       diff = true;
     } else {
-      if (pkg_a->id != pkg_b->id) {
+      if (package_a_iter->id != package_b_iter->id) {
         std::stringstream str_stream;
-        str_stream << "package '" << pkg_a->name << "' has different id (";
+        str_stream << "package '" << package_a_iter->name << "' has different id (";
-        if (pkg_b->id) {
+        if (package_b_iter->id) {
-          str_stream << "0x" << std::hex << pkg_b->id.value();
+          str_stream << "0x" << std::hex << package_b_iter->id.value();
         } else {
           str_stream << "none";
         }
         str_stream << " vs ";
-        if (pkg_a->id) {
+        if (package_a_iter->id) {
-          str_stream << "0x" << std::hex << pkg_a->id.value();
+          str_stream << "0x" << std::hex << package_b_iter->id.value();
         } else {
           str_stream << "none";
         }
@@ -316,20 +323,10 @@ static bool EmitResourceTableDiff(IAaptContext* context, LoadedApk* apk_a, Loade
         EmitDiffLine(apk_b->GetSource(), str_stream.str());
         diff = true;
       }
-      diff |= EmitResourcePackageDiff(context, apk_a, pkg_a.get(), apk_b, pkg_b);
+      diff |= EmitResourcePackageDiff(context, apk_a, *package_a_iter, apk_b, *package_b_iter);
     }
   }
 
-  // Check for any newly added packages.
-  for (std::unique_ptr<ResourceTablePackage>& pkg_b : table_b->packages) {
-    ResourceTablePackage* pkg_a = table_a->FindPackage(pkg_b->name);
-    if (!pkg_a) {
-      std::stringstream str_stream;
-      str_stream << "new package " << pkg_b->name;
-      EmitDiffLine(apk_b->GetSource(), str_stream.str());
-      diff = true;
-    }
-  }
   return diff;
 }
 

tools/aapt2/cmd/Link.cpp

@@ -25,6 +25,7 @@
 #include <vector>
 
 #include "android-base/errors.h"
+#include "android-base/expected.h"
 #include "android-base/file.h"
 #include "android-base/stringprintf.h"
 #include "androidfw/Locale.h"
@@ -74,10 +75,25 @@
 using ::aapt::io::FileInputStream;
 using ::android::ConfigDescription;
 using ::android::StringPiece;
+using ::android::base::expected;
 using ::android::base::StringPrintf;
+using ::android::base::unexpected;
 
 namespace aapt {
 
+namespace {
+
+expected<ResourceTablePackage*, const char*> GetStaticLibraryPackage(ResourceTable* table) {
+  // Resource tables built by aapt2 always contain one package. This is a post condition of
+  // VerifyNoExternalPackages.
+  if (table->packages.size() != 1u) {
+    return unexpected("static library contains more than one package");
+  }
+  return table->packages.back().get();
+}
+
+}  // namespace
+
 constexpr uint8_t kAndroidPackageId = 0x01;
 
 class LinkContext : public IAaptContext {
@@ -633,13 +649,18 @@ bool ResourceFileFlattener::Flatten(ResourceTable* table, IArchiveWriter* archiv
               const ResourceFile& file = doc->file;
               dst_path = ResourceUtils::BuildResourceFileName(file, context_->GetNameMangler());
 
-              std::unique_ptr<FileReference> file_ref =
+              auto file_ref =
                   util::make_unique<FileReference>(table->string_pool.MakeRef(dst_path));
               file_ref->SetSource(doc->file.source);
+
               // Update the output format of this XML file.
               file_ref->type = XmlFileTypeForOutputFormat(options_.output_format);
-              if (!table->AddResourceMangled(file.name, file.config, {}, std::move(file_ref),
+              bool result = table->AddResource(NewResourceBuilder(file.name)
-                                             context_->GetDiagnostics())) {
+                                                   .SetValue(std::move(file_ref), file.config)
+                                                   .SetAllowMangled(true)
+                                                   .Build(),
+                                               context_->GetDiagnostics());
+              if (!result) {
                 return false;
               }
             }
@@ -842,18 +863,15 @@ class Linker {
         ResourceTable* table = static_apk->GetResourceTable();
 
         // If we are using --no-static-lib-packages, we need to rename the package of this table to
-        // our compilation package.
+        // our compilation package so the symbol package name does not get mangled into the entry
-        if (options_.no_static_lib_packages) {
+        // name.
-          // Since package names can differ, and multiple packages can exist in a ResourceTable,
+        if (options_.no_static_lib_packages && !table->packages.empty()) {
-          // we place the requirement that all static libraries are built with the package
+          auto lib_package_result = GetStaticLibraryPackage(table);
-          // ID 0x7f. So if one is not found, this is an error.
+          if (!lib_package_result.has_value()) {
-          if (ResourceTablePackage* pkg = table->FindPackageById(kAppPackageId)) {
+            context_->GetDiagnostics()->Error(DiagMessage(path) << lib_package_result.error());
-            pkg->name = context_->GetCompilationPackage();
-          } else {
-            context_->GetDiagnostics()->Error(DiagMessage(path)
-                                              << "no package with ID 0x7f found in static library");
             return false;
           }
+          lib_package_result.value()->name = context_->GetCompilationPackage();
         }
 
         context_->GetExternalSymbols()->AppendSource(
@@ -982,8 +1000,7 @@ class Linker {
   // stripped, or there is an error and false is returned.
   bool VerifyNoExternalPackages() {
     auto is_ext_package_func = [&](const std::unique_ptr<ResourceTablePackage>& pkg) -> bool {
-      return context_->GetCompilationPackage() != pkg->name || !pkg->id ||
+      return context_->GetCompilationPackage() != pkg->name;
-             pkg->id.value() != context_->GetPackageId();
     };
 
     bool error = false;
@@ -1027,19 +1044,11 @@ class Linker {
   bool VerifyNoIdsSet() {
     for (const auto& package : final_table_.packages) {
       for (const auto& type : package->types) {
-        if (type->id) {
-          context_->GetDiagnostics()->Error(DiagMessage() << "type " << type->type << " has ID "
-                                                          << StringPrintf("%02x", type->id.value())
-                                                          << " assigned");
-          return false;
-        }
-
         for (const auto& entry : type->entries) {
           if (entry->id) {
             ResourceNameRef res_name(package->name, type->type, entry->name);
-            context_->GetDiagnostics()->Error(
+            context_->GetDiagnostics()->Error(DiagMessage() << "resource " << res_name << " has ID "
-                DiagMessage() << "entry " << res_name << " has ID "
+                                                            << entry->id.value() << " assigned");
-                              << StringPrintf("%02x", entry->id.value()) << " assigned");
             return false;
           }
         }
@@ -1313,12 +1322,17 @@ class Linker {
     }
 
     ResourceTable* table = apk->GetResourceTable();
-    ResourceTablePackage* pkg = table->FindPackageById(kAppPackageId);
+    if (table->packages.empty()) {
-    if (!pkg) {
+      return true;
-      context_->GetDiagnostics()->Error(DiagMessage(input) << "static library has no package");
+    }
+
+    auto lib_package_result = GetStaticLibraryPackage(table);
+    if (!lib_package_result.has_value()) {
+      context_->GetDiagnostics()->Error(DiagMessage(input) << lib_package_result.error());
       return false;
     }
 
+    ResourceTablePackage* pkg = lib_package_result.value();
     bool result;
     if (options_.no_static_lib_packages) {
       // Merge all resources as if they were in the compilation package. This is the old behavior
@@ -1365,11 +1379,11 @@ class Linker {
         res_name = mangled_name.value();
       }
 
-      std::unique_ptr<Id> id = util::make_unique<Id>();
+      auto id = util::make_unique<Id>();
       id->SetSource(source.WithLine(exported_symbol.line));
-      bool result =
+      bool result = final_table_.AddResource(
-          final_table_.AddResourceMangled(res_name, ConfigDescription::DefaultConfig(),
+          NewResourceBuilder(res_name).SetValue(std::move(id)).SetAllowMangled(true).Build(),
-                                          std::string(), std::move(id), context_->GetDiagnostics());
+          context_->GetDiagnostics());
       if (!result) {
         return false;
       }
@@ -1750,7 +1764,7 @@ class Linker {
     // anything else being generated, which includes the Java classes.
     // If required, the package name is modifed before flattening, and then modified back
     // to its original name.
-    ResourceTablePackage* package_to_rewrite = nullptr;
+    std::optional<ResourceTablePackageView> package_to_rewrite;
     // Pre-O, the platform treats negative resource IDs [those with a package ID of 0x80
     // or higher] as invalid. In order to work around this limitation, we allow the use
     // of traditionally reserved resource IDs [those between 0x02 and 0x7E]. Allow the
@@ -1764,10 +1778,11 @@ class Linker {
       // The base APK is included, and this is a feature split. If the base package is
       // the same as this package, then we are building an old style Android Instant Apps feature
       // split and must apply this workaround to avoid requiring namespaces support.
-      package_to_rewrite = table->FindPackage(context_->GetCompilationPackage());
+      auto table_view = table->GetPartitionedView();
-      if (package_to_rewrite != nullptr) {
+      if (!table_view.packages.empty() &&
+          table_view.packages.back().name == context_->GetCompilationPackage()) {
+        package_to_rewrite = std::move(table_view.packages.back());
         CHECK_EQ(1u, table->packages.size()) << "can't change name of package when > 1 package";
-
         std::string new_package_name =
             StringPrintf("%s.%s", package_to_rewrite->name.c_str(),
                          app_info_.split_name.value_or_default("feature").c_str());
@@ -1783,7 +1798,7 @@ class Linker {
 
     bool success = FlattenTable(table, options_.output_format, writer);
 
-    if (package_to_rewrite != nullptr) {
+    if (package_to_rewrite.has_value()) {
       // Change the name back.
       package_to_rewrite->name = context_->GetCompilationPackage();
       if (package_to_rewrite->id) {
@@ -1925,8 +1940,7 @@ class Linker {
             for (auto& entry : type->entries) {
               ResourceName name(package->name, type->type, entry->name);
               // The IDs are guaranteed to exist.
-              options_.stable_id_map[std::move(name)] =
+              options_.stable_id_map[std::move(name)] = entry->id.value();
-                  ResourceId(package->id.value(), type->id.value(), entry->id.value());
             }
           }
         }

tools/aapt2/cmd/Link_test.cpp

@@ -47,6 +47,8 @@ TEST_F(LinkTest, RemoveRawXmlStrings) {
   // Load the binary xml tree
   android::ResXMLTree tree;
   std::unique_ptr<LoadedApk> apk = LoadedApk::LoadApkFromPath(out_apk, &diag);
+  ASSERT_THAT(apk, Ne(nullptr));
+
   std::unique_ptr<io::IData> data = OpenFileAsData(apk.get(), "res/xml/test.xml");
   ASSERT_THAT(data, Ne(nullptr));
   AssertLoadXml(apk.get(), data.get(), &tree);
@@ -73,6 +75,8 @@ TEST_F(LinkTest, KeepRawXmlStrings) {
   // Load the binary xml tree
   android::ResXMLTree tree;
   std::unique_ptr<LoadedApk> apk = LoadedApk::LoadApkFromPath(out_apk, &diag);
+  ASSERT_THAT(apk, Ne(nullptr));
+
   std::unique_ptr<io::IData> data = OpenFileAsData(apk.get(), "res/xml/test.xml");
   ASSERT_THAT(data, Ne(nullptr));
   AssertLoadXml(apk.get(), data.get(), &tree);
@@ -208,6 +212,8 @@ TEST_F(LinkTest, OverlayStyles) {
   ASSERT_TRUE(Link(link_args, compiled_files_dir, &diag));
 
   std::unique_ptr<LoadedApk> apk = LoadedApk::LoadApkFromPath(out_apk, &diag);
+  ASSERT_THAT(apk, Ne(nullptr));
+
   const Style* actual_style = test::GetValue<Style>(
       apk->GetResourceTable(), std::string(kDefaultPackageName) + ":style/MyStyle");
   ASSERT_NE(actual_style, nullptr);
@@ -250,6 +256,8 @@ TEST_F(LinkTest, OverrideStylesInsteadOfOverlaying) {
   ASSERT_TRUE(Link(link_args, compiled_files_dir, &diag));
 
   std::unique_ptr<LoadedApk> apk = LoadedApk::LoadApkFromPath(out_apk, &diag);
+  ASSERT_THAT(apk, Ne(nullptr));
+
   const Style* actual_style = test::GetValue<Style>(
       apk->GetResourceTable(), std::string(kDefaultPackageName) + ":style/MyStyle");
   ASSERT_NE(actual_style, nullptr);

tools/aapt2/compile/IdAssigner.cpp

@@ -17,76 +17,109 @@
 #include "compile/IdAssigner.h"
 
 #include <map>
+#include <unordered_map>
 
+#include "android-base/expected.h"
 #include "android-base/logging.h"
 
 #include "ResourceTable.h"
 #include "process/IResourceTableConsumer.h"
 #include "util/Util.h"
 
+using android::base::expected;
+using android::base::unexpected;
+
 namespace aapt {
 
-/**
+namespace {
- * Assigns the intended ID to the ResourceTablePackage, ResourceTableType, and
+template <typename T>
- * ResourceEntry,
+using Result = expected<T, std::string>;
- * as long as there is no existing ID or the ID is the same.
- */
-static bool AssignId(IDiagnostics* diag, const ResourceId& id,
-                     const ResourceName& name, ResourceTablePackage* pkg,
-                     ResourceTableType* type, ResourceEntry* entry) {
-  if (pkg->id.value() == id.package_id()) {
-    if (!type->id || type->id.value() == id.type_id()) {
-      type->id = id.type_id();
 
-      if (!entry->id || entry->id.value() == id.entry_id()) {
+template <typename Id, typename Key>
-        entry->id = id.entry_id();
+struct NextIdFinder {
-        return true;
+  explicit NextIdFinder(Id start_id = 0u) : next_id_(start_id){};
-      }
+
-    }
+  // Attempts to reserve an identifier for the specified key.
+  // If the identifier is already reserved by a different key, an error message is returned.
+  // Reserving identifiers must be completed before `NextId` is called for the first time.
+  Result<Id> ReserveId(Key key, Id id);
+
+  // Retrieves the next available identifier that has not been reserved.
+  std::optional<Id> NextId();
+
+ private:
+  // Attempts to set `next_id_` to the next available identifier that has not been reserved.
+  // Returns whether there were any available identifiers.
+  std::optional<Id> SkipToNextAvailableId();
+
+  Id next_id_;
+  bool next_id_called_ = false;
+  bool exhausted_ = false;
+  std::map<Id, Key> pre_assigned_ids_;
+  typename std::map<Id, Key>::iterator next_preassigned_id_;
+};
+
+struct TypeGroup {
+  explicit TypeGroup(uint8_t package_id, uint8_t type_id)
+      : package_id_(package_id), type_id_(type_id){};
+
+  // Attempts to reserve the resource id for the specified resource name.
+  // If the id is already reserved by a different name, an error message is returned.
+  // Reserving identifiers must be completed before `NextId` is called for the first time.
+  Result<std::monostate> ReserveId(const ResourceName& name, ResourceId id);
+
+  // Retrieves the next available resource id that has not been reserved.
+  Result<ResourceId> NextId();
+
+ private:
+  uint8_t package_id_;
+  uint8_t type_id_;
+  NextIdFinder<uint16_t, ResourceName> next_entry_id_;
+};
+
+struct IdAssignerContext {
+  IdAssignerContext(std::string package_name, uint8_t package_id)
+      : package_name_(std::move(package_name)), package_id_(package_id) {
   }
 
-  const ResourceId existing_id(pkg->id.value(), type->id ? type->id.value() : 0,
+  // Attempts to reserve the resource id for the specified resource name.
-                               entry->id ? entry->id.value() : 0);
+  // Returns whether the id was reserved successfully.
-  diag->Error(DiagMessage() << "can't assign ID " << id << " to resource "
+  // Reserving identifiers must be completed before `NextId` is called for the first time.
-                            << name << " with conflicting ID " << existing_id);
+  bool ReserveId(const ResourceName& name, ResourceId id, IDiagnostics* diag);
-  return false;
+
-}
+  // Retrieves the next available resource id that has not been reserved.
+  std::optional<ResourceId> NextId(const ResourceName& name, IDiagnostics* diag);
+
+ private:
+  std::string package_name_;
+  uint8_t package_id_;
+  std::map<ResourceType, TypeGroup> types_;
+  NextIdFinder<uint8_t, ResourceType> type_id_finder_ = NextIdFinder<uint8_t, ResourceType>(1);
+};
+
+}  // namespace
 
 bool IdAssigner::Consume(IAaptContext* context, ResourceTable* table) {
-  std::map<ResourceId, ResourceName> assigned_ids;
+  IdAssignerContext assigned_ids(context->GetCompilationPackage(), context->GetPackageId());
-
   for (auto& package : table->packages) {
-    CHECK(bool(package->id)) << "packages must have manually assigned IDs";
-
     for (auto& type : package->types) {
       for (auto& entry : type->entries) {
         const ResourceName name(package->name, type->type, entry->name);
+        if (entry->id) {
+          if (!assigned_ids.ReserveId(name, entry->id.value(), context->GetDiagnostics())) {
+            return false;
+          }
+        }
 
         if (assigned_id_map_) {
           // Assign the pre-assigned stable ID meant for this resource.
           const auto iter = assigned_id_map_->find(name);
           if (iter != assigned_id_map_->end()) {
             const ResourceId assigned_id = iter->second;
-            const bool result =
+            if (!assigned_ids.ReserveId(name, assigned_id, context->GetDiagnostics())) {
-                AssignId(context->GetDiagnostics(), assigned_id, name,
-                         package.get(), type.get(), entry.get());
-            if (!result) {
               return false;
             }
-          }
+            entry->id = assigned_id;
-        }
-
-        if (package->id && type->id && entry->id) {
-          // If the ID is set for this resource, then reserve it.
-          ResourceId resource_id(package->id.value(), type->id.value(),
-                                 entry->id.value());
-          auto result = assigned_ids.insert({resource_id, name});
-          const ResourceName& existing_name = result.first->second;
-          if (!result.second) {
-            context->GetDiagnostics()->Error(
-                DiagMessage() << "resource " << name << " has same ID "
-                              << resource_id << " as " << existing_name);
-            return false;
           }
         }
       }
@@ -94,125 +127,162 @@ bool IdAssigner::Consume(IAaptContext* context, ResourceTable* table) {
   }
 
   if (assigned_id_map_) {
-    // Reserve all the IDs mentioned in the stable ID map. That way we won't
+    // Reserve all the IDs mentioned in the stable ID map. That way we won't assig IDs that were
-    // assign
+    // listed in the map if they don't exist in the table.
-    // IDs that were listed in the map if they don't exist in the table.
     for (const auto& stable_id_entry : *assigned_id_map_) {
       const ResourceName& pre_assigned_name = stable_id_entry.first;
       const ResourceId& pre_assigned_id = stable_id_entry.second;
-      auto result = assigned_ids.insert({pre_assigned_id, pre_assigned_name});
+      if (!assigned_ids.ReserveId(pre_assigned_name, pre_assigned_id, context->GetDiagnostics())) {
-      const ResourceName& existing_name = result.first->second;
-      if (!result.second && existing_name != pre_assigned_name) {
-        context->GetDiagnostics()->Error(
-            DiagMessage() << "stable ID " << pre_assigned_id << " for resource "
-                          << pre_assigned_name
-                          << " is already taken by resource " << existing_name);
         return false;
       }
     }
   }
 
-  // Assign any resources without IDs the next available ID. Gaps will be filled
+  // Assign any resources without IDs the next available ID. Gaps will be filled if possible,
-  // if possible,
   // unless those IDs have been reserved.
-
-  const auto assigned_ids_iter_end = assigned_ids.end();
   for (auto& package : table->packages) {
-    CHECK(bool(package->id)) << "packages must have manually assigned IDs";
-
-    // Build a half filled ResourceId object, which will be used to find the
-    // closest matching
-    // reserved ID in the assignedId map. From that point the next available
-    // type ID can be
-    // found.
-    ResourceId resource_id(package->id.value(), 0, 0);
-    uint8_t next_expected_type_id = 1;
-
-    // Find the closest matching ResourceId that is <= the one with only the
-    // package set.
-    auto next_type_iter = assigned_ids.lower_bound(resource_id);
     for (auto& type : package->types) {
-      if (!type->id) {
-        // We need to assign a type ID. Iterate over the reserved IDs until we
-        // find
-        // some type ID that is a distance of 2 greater than the last one we've
-        // seen.
-        // That means there is an available type ID between these reserved IDs.
-        while (next_type_iter != assigned_ids_iter_end) {
-          if (next_type_iter->first.package_id() != package->id.value()) {
-            break;
-          }
-
-          const uint8_t type_id = next_type_iter->first.type_id();
-          if (type_id > next_expected_type_id) {
-            // There is a gap in the type IDs, so use the missing one.
-            type->id = next_expected_type_id++;
-            break;
-          }
-
-          // Set our expectation to be the next type ID after the reserved one
-          // we
-          // just saw.
-          next_expected_type_id = type_id + 1;
-
-          // Move to the next reserved ID.
-          ++next_type_iter;
-        }
-
-        if (!type->id) {
-          // We must have hit the end of the reserved IDs and not found a gap.
-          // That means the next ID is available.
-          type->id = next_expected_type_id++;
-        }
-      }
-
-      resource_id = ResourceId(package->id.value(), type->id.value(), 0);
-      uint16_t next_expected_entry_id = 0;
-
-      // Find the closest matching ResourceId that is <= the one with only the
-      // package
-      // and type set.
-      auto next_entry_iter = assigned_ids.lower_bound(resource_id);
       for (auto& entry : type->entries) {
-        if (!entry->id) {
+        const ResourceName name(package->name, type->type, entry->name);
-          // We need to assign an entry ID. Iterate over the reserved IDs until
+        if (entry->id) {
-          // we find
+          continue;
-          // some entry ID that is a distance of 2 greater than the last one
-          // we've seen.
-          // That means there is an available entry ID between these reserved
-          // IDs.
-          while (next_entry_iter != assigned_ids_iter_end) {
-            if (next_entry_iter->first.package_id() != package->id.value() ||
-                next_entry_iter->first.type_id() != type->id.value()) {
-              break;
-            }
-
-            const uint16_t entry_id = next_entry_iter->first.entry_id();
-            if (entry_id > next_expected_entry_id) {
-              // There is a gap in the entry IDs, so use the missing one.
-              entry->id = next_expected_entry_id++;
-              break;
-            }
-
-            // Set our expectation to be the next type ID after the reserved one
-            // we
-            // just saw.
-            next_expected_entry_id = entry_id + 1;
-
-            // Move to the next reserved entry ID.
-            ++next_entry_iter;
-          }
-
-          if (!entry->id) {
-            // We must have hit the end of the reserved IDs and not found a gap.
-            // That means the next ID is available.
-            entry->id = next_expected_entry_id++;
         }
+        auto id = assigned_ids.NextId(name, context->GetDiagnostics());
+        if (!id.has_value()) {
+          return false;
         }
+        entry->id = id.value();
       }
     }
   }
   return true;
 }
 
+namespace {
+template <typename Id, typename Key>
+Result<Id> NextIdFinder<Id, Key>::ReserveId(Key key, Id id) {
+  CHECK(!next_id_called_) << "ReserveId cannot be called after NextId";
+  auto assign_result = pre_assigned_ids_.emplace(id, key);
+  if (!assign_result.second && assign_result.first->second != key) {
+    std::stringstream error;
+    error << "ID " << id << " is already assigned to " << assign_result.first->second;
+    return unexpected(error.str());
+  }
+  return id;
+}
+
+template <typename Id, typename Key>
+std::optional<Id> NextIdFinder<Id, Key>::NextId() {
+  if (!next_id_called_) {
+    next_id_called_ = true;
+    next_preassigned_id_ = pre_assigned_ids_.begin();
+  }
+  return SkipToNextAvailableId();
+}
+
+template <typename Id, typename Key>
+std::optional<Id> NextIdFinder<Id, Key>::SkipToNextAvailableId() {
+  if (exhausted_) {
+    return {};
+  }
+  while (next_preassigned_id_ != pre_assigned_ids_.end()) {
+    if (next_preassigned_id_->first == next_id_) {
+      if (next_id_ == std::numeric_limits<Id>::max()) {
+        // The last identifier was reserved so there are no more available identifiers.
+        exhausted_ = true;
+        return {};
+      }
+      ++next_id_;
+      ++next_preassigned_id_;
+      continue;
+    }
+    CHECK(next_preassigned_id_->first > next_id_) << "Preassigned IDs are not in sorted order";
+    break;
+  }
+  if (next_id_ == std::numeric_limits<Id>::max()) {
+    // There are no more identifiers after this one, but this one is still available so return it.
+    exhausted_ = true;
+  }
+  return next_id_++;
+}
+
+Result<std::monostate> TypeGroup::ReserveId(const ResourceName& name, ResourceId id) {
+  if (type_id_ != id.type_id()) {
+    // Currently there cannot be multiple type ids for a single type.
+    std::stringstream error;
+    error << "type '" << name.type << "' already has ID " << id.type_id();
+    return unexpected(error.str());
+  }
+
+  auto assign_result = next_entry_id_.ReserveId(name, id.entry_id());
+  if (!assign_result.has_value()) {
+    std::stringstream error;
+    error << "entry " << assign_result.error();
+    return unexpected(error.str());
+  }
+  return {};
+}
+
+Result<ResourceId> TypeGroup::NextId() {
+  auto entry_id = next_entry_id_.NextId();
+  if (!entry_id.has_value()) {
+    std::stringstream error;
+    error << "resource type ID has exceeded the maximum number of resource entries ("
+          << (std::numeric_limits<uint16_t>::max() + 1u) << ")";
+    return unexpected(error.str());
+  }
+  return ResourceId(package_id_, type_id_, entry_id.value());
+}
+
+bool IdAssignerContext::ReserveId(const ResourceName& name, ResourceId id, IDiagnostics* diag) {
+  if (package_id_ != id.package_id()) {
+    diag->Error(DiagMessage() << "can't assign ID " << id << " to resource " << name
+                              << " because package already has ID " << id.package_id());
+    return false;
+  }
+
+  auto type = types_.find(name.type);
+  if (type == types_.end()) {
+    // The type has not been assigned an id yet. Ensure that the specified id is not being used by
+    // another type.
+    auto assign_result = type_id_finder_.ReserveId(name.type, id.type_id());
+    if (!assign_result.has_value()) {
+      diag->Error(DiagMessage() << "can't assign ID " << id << " to resource " << name
+                                << " because type " << assign_result.error());
+      return false;
+    }
+    type = types_.emplace(name.type, TypeGroup(package_id_, id.type_id())).first;
+  }
+
+  auto assign_result = type->second.ReserveId(name, id);
+  if (!assign_result.has_value()) {
+    diag->Error(DiagMessage() << "can't assign ID " << id << " to resource " << name << " because "
+                              << assign_result.error());
+    return false;
+  }
+
+  return true;
+}
+
+std::optional<ResourceId> IdAssignerContext::NextId(const ResourceName& name, IDiagnostics* diag) {
+  // The package name is not known during the compile stage.
+  // Resources without a package name are considered a part of the app being linked.
+  CHECK(name.package.empty() || name.package == package_name_);
+  auto type = types_.find(name.type);
+  if (type == types_.end()) {
+    auto next_type_id = type_id_finder_.NextId();
+    CHECK(next_type_id.has_value()) << "resource type IDs allocated have exceeded maximum (256)";
+    type = types_.emplace(name.type, TypeGroup(package_id_, next_type_id.value())).first;
+  }
+
+  auto assign_result = type->second.NextId();
+  if (!assign_result.has_value()) {
+    diag->Error(DiagMessage() << "can't assign resource ID to resource " << name << " because "
+                              << assign_result.error());
+    return {};
+  }
+  return assign_result.value();
+}
+}  // namespace
+
 }  // namespace aapt

tools/aapt2/compile/IdAssigner_test.cpp

@@ -20,26 +20,29 @@
 
 namespace aapt {
 
+struct IdAssignerTests : public ::testing::Test {
+  void SetUp() override {
+    context = test::ContextBuilder().SetCompilationPackage("android").SetPackageId(0x01).Build();
+  }
+  std::unique_ptr<IAaptContext> context;
+};
+
 ::testing::AssertionResult VerifyIds(ResourceTable* table);
 
-TEST(IdAssignerTest, AssignIds) {
+TEST_F(IdAssignerTests, AssignIds) {
-  std::unique_ptr<ResourceTable> table = test::ResourceTableBuilder()
+  auto table = test::ResourceTableBuilder()
                    .AddSimple("android:attr/foo")
                    .AddSimple("android:attr/bar")
                    .AddSimple("android:id/foo")
-                                             .SetPackageId("android", 0x01)
                    .Build();
-
-  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
   IdAssigner assigner;
 
   ASSERT_TRUE(assigner.Consume(context.get(), table.get()));
   ASSERT_TRUE(VerifyIds(table.get()));
 }
 
-TEST(IdAssignerTest, AssignIdsWithReservedIds) {
+TEST_F(IdAssignerTests, AssignIdsWithReservedIds) {
-  std::unique_ptr<ResourceTable> table =
+  auto table = test::ResourceTableBuilder()
-      test::ResourceTableBuilder()
                    .AddSimple("android:id/foo", ResourceId(0x01010000))
                    .AddSimple("android:dimen/two")
                    .AddSimple("android:integer/three")
@@ -48,14 +51,9 @@ TEST(IdAssignerTest, AssignIdsWithReservedIds) {
                    .AddSimple("android:attr/foo", ResourceId(0x01040006))
                    .AddSimple("android:attr/bar")
                    .AddSimple("android:attr/baz")
-          .AddSimple("app:id/biz")
-          .SetPackageId("android", 0x01)
-          .SetPackageId("app", 0x7f)
                    .Build();
 
-  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
   IdAssigner assigner;
-
   ASSERT_TRUE(assigner.Consume(context.get(), table.get()));
   ASSERT_TRUE(VerifyIds(table.get()));
 
@@ -65,12 +63,12 @@ TEST(IdAssignerTest, AssignIdsWithReservedIds) {
 
   maybe_result = table->FindResource(test::ParseNameOrDie("android:dimen/two"));
   ASSERT_TRUE(maybe_result);
-  EXPECT_EQ(make_value<uint8_t>(2), maybe_result.value().type->id);
+  EXPECT_EQ(make_value<ResourceId>(0x01020000), maybe_result.value().entry->id);
 
   maybe_result =
       table->FindResource(test::ParseNameOrDie("android:integer/three"));
   ASSERT_TRUE(maybe_result);
-  EXPECT_EQ(make_value<uint8_t>(3), maybe_result.value().type->id);
+  EXPECT_EQ(make_value<ResourceId>(0x01030000), maybe_result.value().entry->id);
 
   // Expect to bypass the reserved 0x0104XXXX IDs and use the next 0x0105XXXX
   // IDs.
@@ -78,103 +76,126 @@ TEST(IdAssignerTest, AssignIdsWithReservedIds) {
   maybe_result =
       table->FindResource(test::ParseNameOrDie("android:string/five"));
   ASSERT_TRUE(maybe_result);
-  EXPECT_EQ(make_value<uint8_t>(5), maybe_result.value().type->id);
+  EXPECT_EQ(make_value<ResourceId>(0x01050000), maybe_result.value().entry->id);
 
   // Expect to fill in the gaps between 0x01040000 and 0x01040006.
 
   maybe_result = table->FindResource(test::ParseNameOrDie("android:attr/bar"));
   ASSERT_TRUE(maybe_result);
-  EXPECT_EQ(make_value<uint16_t>(1), maybe_result.value().entry->id);
+  EXPECT_EQ(make_value<ResourceId>(0x01040001), maybe_result.value().entry->id);
 
   maybe_result = table->FindResource(test::ParseNameOrDie("android:attr/baz"));
   ASSERT_TRUE(maybe_result);
-  EXPECT_EQ(make_value<uint16_t>(2), maybe_result.value().entry->id);
+  EXPECT_EQ(make_value<ResourceId>(0x01040002), maybe_result.value().entry->id);
 }
 
-TEST(IdAssignerTest, FailWhenNonUniqueIdsAssigned) {
+TEST_F(IdAssignerTests, FailWhenNonUniqueIdsAssigned) {
-  std::unique_ptr<ResourceTable> table =
+  auto table = test::ResourceTableBuilder()
-      test::ResourceTableBuilder()
                    .AddSimple("android:attr/foo", ResourceId(0x01040006))
                    .AddSimple("android:attr/bar", ResourceId(0x01040006))
-          .SetPackageId("android", 0x01)
-          .SetPackageId("app", 0x7f)
                    .Build();
-
-  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
   IdAssigner assigner;
-
   ASSERT_FALSE(assigner.Consume(context.get(), table.get()));
 }
 
-TEST(IdAssignerTest, AssignIdsWithIdMap) {
+TEST_F(IdAssignerTests, AssignIdsWithIdMap) {
-  std::unique_ptr<ResourceTable> table = test::ResourceTableBuilder()
+  auto table = test::ResourceTableBuilder()
                    .AddSimple("android:attr/foo")
                    .AddSimple("android:attr/bar")
-                                             .SetPackageId("android", 0x01)
                    .Build();
-
-  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
   std::unordered_map<ResourceName, ResourceId> id_map = {
       {test::ParseNameOrDie("android:attr/foo"), ResourceId(0x01010002)}};
   IdAssigner assigner(&id_map);
   ASSERT_TRUE(assigner.Consume(context.get(), table.get()));
   ASSERT_TRUE(VerifyIds(table.get()));
-  Maybe<ResourceTable::SearchResult> result =
+  auto result = table->FindResource(test::ParseNameOrDie("android:attr/foo"));
-      table->FindResource(test::ParseNameOrDie("android:attr/foo"));
   ASSERT_TRUE(result);
 
   const ResourceTable::SearchResult& search_result = result.value();
-  EXPECT_EQ(make_value<uint8_t>(0x01), search_result.package->id);
+  EXPECT_EQ(make_value<ResourceId>(0x01010002), search_result.entry->id);
-  EXPECT_EQ(make_value<uint8_t>(0x01), search_result.type->id);
+}
-  EXPECT_EQ(make_value<uint16_t>(0x0002), search_result.entry->id);
+
+TEST_F(IdAssignerTests, UseAllEntryIds) {
+  ResourceTable table;
+  const size_t max_entry_id = std::numeric_limits<uint16_t>::max();
+  for (size_t i = 0; i <= max_entry_id; i++) {
+    ASSERT_TRUE(
+        table.AddResource(NewResourceBuilder("android:attr/res" + std::to_string(i)).Build(),
+                          context->GetDiagnostics()));
+  }
+  IdAssigner assigner;
+  ASSERT_TRUE(assigner.Consume(context.get(), &table));
+}
+
+TEST_F(IdAssignerTests, ExaustEntryIds) {
+  ResourceTable table;
+  const size_t max_entry_id = std::numeric_limits<uint16_t>::max() + 1u;
+  for (size_t i = 0; i <= max_entry_id; i++) {
+    ASSERT_TRUE(
+        table.AddResource(NewResourceBuilder("android:attr/res" + std::to_string(i)).Build(),
+                          context->GetDiagnostics()));
+  }
+  IdAssigner assigner;
+  ASSERT_FALSE(assigner.Consume(context.get(), &table));
+}
+
+TEST_F(IdAssignerTests, ExaustEntryIdsLastIdIsPublic) {
+  ResourceTable table;
+  ASSERT_TRUE(table.AddResource(NewResourceBuilder("android:attr/res").SetId(0x0101ffff).Build(),
+                                context->GetDiagnostics()));
+  const size_t max_entry_id = std::numeric_limits<uint16_t>::max();
+  for (size_t i = 0; i <= max_entry_id; i++) {
+    ASSERT_TRUE(
+        table.AddResource(NewResourceBuilder("android:attr/res" + std::to_string(i)).Build(),
+                          context->GetDiagnostics()));
+  }
+  IdAssigner assigner;
+  ASSERT_FALSE(assigner.Consume(context.get(), &table));
 }
 
 ::testing::AssertionResult VerifyIds(ResourceTable* table) {
   std::set<uint8_t> package_ids;
-  for (auto& package : table->packages) {
+  auto table_view = table->GetPartitionedView();
-    if (!package->id) {
+  for (auto& package : table_view.packages) {
-      return ::testing::AssertionFailure() << "package " << package->name
+    if (!package.id) {
-                                           << " has no ID";
+      return ::testing::AssertionFailure() << "package " << package.name << " has no ID";
     }
 
-    if (!package_ids.insert(package->id.value()).second) {
+    if (!package_ids.insert(package.id.value()).second) {
-      return ::testing::AssertionFailure()
+      return ::testing::AssertionFailure() << "package " << package.name << " has non-unique ID "
-             << "package " << package->name << " has non-unique ID " << std::hex
+                                           << std::hex << (int)package.id.value() << std::dec;
-             << (int)package->id.value() << std::dec;
     }
   }
 
-  for (auto& package : table->packages) {
+  for (auto& package : table_view.packages) {
     std::set<uint8_t> type_ids;
-    for (auto& type : package->types) {
+    for (auto& type : package.types) {
-      if (!type->id) {
+      if (!type.id) {
-        return ::testing::AssertionFailure() << "type " << type->type
-                                             << " of package " << package->name
-                                             << " has no ID";
-      }
-
-      if (!type_ids.insert(type->id.value()).second) {
         return ::testing::AssertionFailure()
-               << "type " << type->type << " of package " << package->name
+               << "type " << type.type << " of package " << package.name << " has no ID";
-               << " has non-unique ID " << std::hex << (int)type->id.value()
+      }
-               << std::dec;
+
+      if (!type_ids.insert(type.id.value()).second) {
+        return ::testing::AssertionFailure()
+               << "type " << type.type << " of package " << package.name << " has non-unique ID "
+               << std::hex << (int)type.id.value() << std::dec;
       }
     }
 
-    for (auto& type : package->types) {
+    for (auto& type : package.types) {
-      std::set<uint16_t> entry_ids;
+      std::set<ResourceId> entry_ids;
-      for (auto& entry : type->entries) {
+      for (auto& entry : type.entries) {
         if (!entry->id) {
           return ::testing::AssertionFailure()
-                 << "entry " << entry->name << " of type " << type->type
+                 << "entry " << entry->name << " of type " << type.type << " of package "
-                 << " of package " << package->name << " has no ID";
+                 << package.name << " has no ID";
         }
 
         if (!entry_ids.insert(entry->id.value()).second) {
           return ::testing::AssertionFailure()
-                 << "entry " << entry->name << " of type " << type->type
+                 << "entry " << entry->name << " of type " << type.type << " of package "
-                 << " of package " << package->name << " has non-unique ID "
+                 << package.name << " has non-unique ID " << std::hex << entry->id.value()
-                 << std::hex << (int)entry->id.value() << std::dec;
+                 << std::dec;
         }
       }
     }

tools/aapt2/compile/PseudolocaleGenerator_test.cpp

@@ -236,13 +236,14 @@ TEST(PseudolocaleGeneratorTest, PseudolocalizeOnlyDefaultConfigs) {
 
 TEST(PseudolocaleGeneratorTest, PluralsArePseudolocalized) {
   std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
-  std::unique_ptr<ResourceTable> table =
+  std::unique_ptr<ResourceTable> table = test::ResourceTableBuilder().Build();
-      test::ResourceTableBuilder().SetPackageId("com.pkg", 0x7F).Build();
   std::unique_ptr<Plural> plural = util::make_unique<Plural>();
   plural->values = {util::make_unique<String>(table->string_pool.MakeRef("zero")),
                     util::make_unique<String>(table->string_pool.MakeRef("one"))};
-  ASSERT_TRUE(table->AddResource(test::ParseNameOrDie("com.pkg:plurals/foo"), ConfigDescription{},
+  ASSERT_TRUE(table->AddResource(NewResourceBuilder(test::ParseNameOrDie("com.pkg:plurals/foo"))
-                                 {}, std::move(plural), context->GetDiagnostics()));
+                                     .SetValue(std::move(plural))
+                                     .Build(),
+                                 context->GetDiagnostics()));
   std::unique_ptr<Plural> expected = util::make_unique<Plural>();
   expected->values = {util::make_unique<String>(table->string_pool.MakeRef("[žéŕö one]")),
                       util::make_unique<String>(table->string_pool.MakeRef("[öñé one]"))};
@@ -252,6 +253,7 @@ TEST(PseudolocaleGeneratorTest, PluralsArePseudolocalized) {
 
   const auto* actual = test::GetValueForConfig<Plural>(table.get(), "com.pkg:plurals/foo",
                                                        test::ParseConfigOrDie("en-rXA"));
+  ASSERT_NE(nullptr, actual);
   EXPECT_TRUE(actual->Equals(expected.get()));
 }
 
@@ -273,11 +275,14 @@ TEST(PseudolocaleGeneratorTest, RespectUntranslateableSections) {
     auto string = util::make_unique<String>(table->string_pool.MakeRef(original_style.str));
     string->untranslatable_sections.push_back(UntranslatableSection{6u, 11u});
 
-    ASSERT_TRUE(table->AddResource(test::ParseNameOrDie("android:string/foo"), ConfigDescription{},
+    ASSERT_TRUE(table->AddResource(NewResourceBuilder(test::ParseNameOrDie("android:string/foo"))
-                                   {} /* product */, std::move(styled_string),
+                                       .SetValue(std::move(styled_string))
+                                       .Build(),
+                                   context->GetDiagnostics()));
+    ASSERT_TRUE(table->AddResource(NewResourceBuilder(test::ParseNameOrDie("android:string/bar"))
+                                       .SetValue(std::move(string))
+                                       .Build(),
                                    context->GetDiagnostics()));
-    ASSERT_TRUE(table->AddResource(test::ParseNameOrDie("android:string/bar"), ConfigDescription{},
-                                   {} /* product */, std::move(string), context->GetDiagnostics()));
   }
 
   PseudolocaleGenerator generator;

tools/aapt2/dump/DumpManifest.cpp

@@ -191,11 +191,9 @@ class ManifestExtractor {
                          const ConfigDescription& config = DefaultConfig()) {
       if (table) {
         for (auto& package : table->packages) {
-          if (package->id && package->id.value() == res_id.package_id()) {
             for (auto& type : package->types) {
-              if (type->id && type->id.value() == res_id.type_id()) {
               for (auto& entry : type->entries) {
-                  if (entry->id && entry->id.value() == res_id.entry_id()) {
+                if (entry->id && entry->id.value() == res_id.id) {
                   if (auto value = BestConfigValue(entry.get(), config)) {
                     return value;
                   }
@@ -204,8 +202,6 @@ class ManifestExtractor {
           }
         }
       }
-        }
-      }
       return nullptr;
     }
 

tools/aapt2/format/binary/BinaryResourceParser.cpp

@@ -192,8 +192,7 @@ bool BinaryResourceParser::ParsePackage(const ResChunk_header* chunk) {
   std::u16string package_name = strcpy16_dtoh((const char16_t*)package_header->name,
                                               arraysize(package_header->name));
 
-  ResourceTablePackage* package =
+  ResourceTablePackage* package = table_->FindOrCreatePackage(util::Utf16ToUtf8(package_name));
-      table_->CreatePackage(util::Utf16ToUtf8(package_name), static_cast<uint8_t>(package_id));
   if (!package) {
     diag_->Error(DiagMessage(source_)
                  << "incompatible package '" << package_name << "' with ID " << package_id);
@@ -232,13 +231,13 @@ bool BinaryResourceParser::ParsePackage(const ResChunk_header* chunk) {
         break;
 
       case android::RES_TABLE_TYPE_SPEC_TYPE:
-        if (!ParseTypeSpec(package, parser.chunk())) {
+        if (!ParseTypeSpec(package, parser.chunk(), package_id)) {
           return false;
         }
         break;
 
       case android::RES_TABLE_TYPE_TYPE:
-        if (!ParseType(package, parser.chunk())) {
+        if (!ParseType(package, parser.chunk(), package_id)) {
           return false;
         }
         break;
@@ -276,7 +275,7 @@ bool BinaryResourceParser::ParsePackage(const ResChunk_header* chunk) {
 }
 
 bool BinaryResourceParser::ParseTypeSpec(const ResourceTablePackage* package,
-                                         const ResChunk_header* chunk) {
+                                         const ResChunk_header* chunk, uint8_t package_id) {
   if (type_pool_.getError() != NO_ERROR) {
     diag_->Error(DiagMessage(source_) << "missing type string pool");
     return false;
@@ -317,14 +316,14 @@ bool BinaryResourceParser::ParseTypeSpec(const ResourceTablePackage* package,
   const uint32_t* type_spec_flags = reinterpret_cast<const uint32_t*>(
       reinterpret_cast<uintptr_t>(type_spec) + util::DeviceToHost16(type_spec->header.headerSize));
   for (size_t i = 0; i < entry_count; i++) {
-    ResourceId id(package->id.value_or_default(0x0), type_spec->id, static_cast<size_t>(i));
+    ResourceId id(package_id, type_spec->id, static_cast<size_t>(i));
     entry_type_spec_flags_[id] = util::DeviceToHost32(type_spec_flags[i]);
   }
   return true;
 }
 
 bool BinaryResourceParser::ParseType(const ResourceTablePackage* package,
-                                     const ResChunk_header* chunk) {
+                                     const ResChunk_header* chunk, uint8_t package_id) {
   if (type_pool_.getError() != NO_ERROR) {
     diag_->Error(DiagMessage(source_) << "missing type string pool");
     return false;
@@ -354,13 +353,9 @@ bool BinaryResourceParser::ParseType(const ResourceTablePackage* package,
   const std::string type_str = util::GetString(type_pool_, type->id - 1);
   const ResourceType* parsed_type = ParseResourceType(type_str);
   if (!parsed_type) {
-    // Be lenient on the name of the type if the table is lenient on resource validation.
+    diag_->Warn(DiagMessage(source_)
-    bool log_error = table_->GetValidateResources();
+                << "invalid type name '" << type_str << "' for type with ID " << type->id);
-    if (log_error) {
+    return true;
-      diag_->Error(DiagMessage(source_) << "invalid type name '" << type_str
-                                        << "' for type with ID " << type->id);
-    }
-    return !log_error;
   }
 
   TypeVariant tv(type);
@@ -372,7 +367,7 @@ bool BinaryResourceParser::ParseType(const ResourceTablePackage* package,
 
     const ResourceName name(package->name, *parsed_type,
                             util::GetString(key_pool_, util::DeviceToHost32(entry->key.index)));
-    const ResourceId res_id(package->id.value(), type->id, static_cast<uint16_t>(it.index()));
+    const ResourceId res_id(package_id, type->id, static_cast<uint16_t>(it.index()));
 
     std::unique_ptr<Value> resource_value;
     if (entry->flags & ResTable_entry::FLAG_COMPLEX) {
@@ -392,17 +387,13 @@ bool BinaryResourceParser::ParseType(const ResourceTablePackage* package,
       return false;
     }
 
-    if (!table_->AddResourceWithIdMangled(name, res_id, config, {}, std::move(resource_value),
+    NewResourceBuilder res_builder(name);
-                                          diag_)) {
+    res_builder.SetValue(std::move(resource_value), config)
-      return false;
+        .SetId(res_id, OnIdConflict::CREATE_ENTRY)
-    }
+        .SetAllowMangled(true);
 
     if (entry->flags & ResTable_entry::FLAG_PUBLIC) {
-      Visibility visibility;
+      res_builder.SetVisibility(Visibility{Visibility::Level::kPublic});
-      visibility.level = Visibility::Level::kPublic;
-      if (!table_->SetVisibilityWithIdMangled(name, visibility, res_id, diag_)) {
-        return false;
-      }
 
       // Erase the ID from the map once processed, so that we don't mark the same symbol more than
       // once.
@@ -415,6 +406,10 @@ bool BinaryResourceParser::ParseType(const ResourceTablePackage* package,
     if (cache_iter == id_index_.end()) {
       id_index_.insert({res_id, name});
     }
+
+    if (!table_->AddResource(res_builder.Build(), diag_)) {
+      return false;
+    }
   }
   return true;
 }
@@ -472,7 +467,12 @@ bool BinaryResourceParser::ParseOverlayable(const ResChunk_header* chunk) {
 
         OverlayableItem overlayable_item(overlayable);
         overlayable_item.policies = policy_header->policy_flags;
-        if (!table_->SetOverlayable(iter->second, overlayable_item, diag_)) {
+        if (!table_->AddResource(NewResourceBuilder(iter->second)
+                                     .SetId(res_id, OnIdConflict::CREATE_ENTRY)
+                                     .SetOverlayable(std::move(overlayable_item))
+                                     .SetAllowMangled(true)
+                                     .Build(),
+                                 diag_)) {
           return false;
         }
       }

tools/aapt2/format/binary/BinaryResourceParser.h

@@ -51,8 +51,10 @@ class BinaryResourceParser {
 
   bool ParseTable(const android::ResChunk_header* chunk);
   bool ParsePackage(const android::ResChunk_header* chunk);
-  bool ParseTypeSpec(const ResourceTablePackage* package, const android::ResChunk_header* chunk);
+  bool ParseTypeSpec(const ResourceTablePackage* package, const android::ResChunk_header* chunk,
-  bool ParseType(const ResourceTablePackage* package, const android::ResChunk_header* chunk);
+                     uint8_t package_id);
+  bool ParseType(const ResourceTablePackage* package, const android::ResChunk_header* chunk,
+                 uint8_t package_id);
   bool ParseLibrary(const android::ResChunk_header* chunk);
   bool ParseOverlayable(const android::ResChunk_header* chunk);
 

tools/aapt2/format/binary/TableFlattener.cpp

@@ -59,35 +59,35 @@ static void strcpy16_htod(uint16_t* dst, size_t len, const StringPiece16& src) {
   dst[i] = 0;
 }
 
-static bool cmp_style_entries(const Style::Entry& a, const Style::Entry& b) {
+static bool cmp_style_entries(const Style::Entry* a, const Style::Entry* b) {
-  if (a.key.id) {
+  if (a->key.id) {
-    if (b.key.id) {
+    if (b->key.id) {
-      return cmp_ids_dynamic_after_framework(a.key.id.value(), b.key.id.value());
+      return cmp_ids_dynamic_after_framework(a->key.id.value(), b->key.id.value());
     }
     return true;
-  } else if (!b.key.id) {
+  } else if (!b->key.id) {
-    return a.key.name.value() < b.key.name.value();
+    return a->key.name.value() < b->key.name.value();
   }
   return false;
 }
 
 struct FlatEntry {
-  ResourceEntry* entry;
+  const ResourceEntry* entry;
-  Value* value;
+  const Value* value;
 
   // The entry string pool index to the entry's name.
   uint32_t entry_key;
 };
 
-class MapFlattenVisitor : public ValueVisitor {
+class MapFlattenVisitor : public ConstValueVisitor {
  public:
-  using ValueVisitor::Visit;
+  using ConstValueVisitor::Visit;
 
   MapFlattenVisitor(ResTable_entry_ext* out_entry, BigBuffer* buffer)
       : out_entry_(out_entry), buffer_(buffer) {
   }
 
-  void Visit(Attribute* attr) override {
+  void Visit(const Attribute* attr) override {
     {
       Reference key = Reference(ResourceId(ResTable_map::ATTR_TYPE));
       BinaryPrimitive val(Res_value::TYPE_INT_DEC, attr->type_mask);
@@ -106,13 +106,13 @@ class MapFlattenVisitor : public ValueVisitor {
       FlattenEntry(&key, &val);
     }
 
-    for (Attribute::Symbol& s : attr->symbols) {
+    for (const Attribute::Symbol& s : attr->symbols) {
       BinaryPrimitive val(s.type, s.value);
       FlattenEntry(&s.symbol, &val);
     }
   }
 
-  void Visit(Style* style) override {
+  void Visit(const Style* style) override {
     if (style->parent) {
       const Reference& parent_ref = style->parent.value();
       CHECK(bool(parent_ref.id)) << "parent has no ID";
@@ -120,21 +120,26 @@ class MapFlattenVisitor : public ValueVisitor {
     }
 
     // Sort the style.
-    std::sort(style->entries.begin(), style->entries.end(), cmp_style_entries);
+    std::vector<const Style::Entry*> sorted_entries;
+    for (const auto& entry : style->entries) {
+      sorted_entries.emplace_back(&entry);
+    }
 
-    for (Style::Entry& entry : style->entries) {
+    std::sort(sorted_entries.begin(), sorted_entries.end(), cmp_style_entries);
-      FlattenEntry(&entry.key, entry.value.get());
+
+    for (const Style::Entry* entry : sorted_entries) {
+      FlattenEntry(&entry->key, entry->value.get());
     }
   }
 
-  void Visit(Styleable* styleable) override {
+  void Visit(const Styleable* styleable) override {
     for (auto& attr_ref : styleable->entries) {
       BinaryPrimitive val(Res_value{});
       FlattenEntry(&attr_ref, &val);
     }
   }
 
-  void Visit(Array* array) override {
+  void Visit(const Array* array) override {
     const size_t count = array->elements.size();
     for (size_t i = 0; i < count; i++) {
       Reference key(android::ResTable_map::ATTR_MIN + i);
@@ -142,7 +147,7 @@ class MapFlattenVisitor : public ValueVisitor {
     }
   }
 
-  void Visit(Plural* plural) override {
+  void Visit(const Plural* plural) override {
     const size_t count = plural->values.size();
     for (size_t i = 0; i < count; i++) {
       if (!plural->values[i]) {
@@ -196,16 +201,16 @@ class MapFlattenVisitor : public ValueVisitor {
  private:
   DISALLOW_COPY_AND_ASSIGN(MapFlattenVisitor);
 
-  void FlattenKey(Reference* key, ResTable_map* out_entry) {
+  void FlattenKey(const Reference* key, ResTable_map* out_entry) {
     CHECK(bool(key->id)) << "key has no ID";
     out_entry->name.ident = util::HostToDevice32(key->id.value().id);
   }
 
-  void FlattenValue(Item* value, ResTable_map* out_entry) {
+  void FlattenValue(const Item* value, ResTable_map* out_entry) {
     CHECK(value->Flatten(&out_entry->value)) << "flatten failed";
   }
 
-  void FlattenEntry(Reference* key, Item* value) {
+  void FlattenEntry(const Reference* key, Item* value) {
     ResTable_map* out_entry = buffer_->NextBlock<ResTable_map>();
     FlattenKey(key, out_entry);
     FlattenValue(value, out_entry);
@@ -226,7 +231,7 @@ struct OverlayableChunk {
 
 class PackageFlattener {
  public:
-  PackageFlattener(IAaptContext* context, ResourceTablePackage* package,
+  PackageFlattener(IAaptContext* context, const ResourceTablePackageView& package,
                    const std::map<size_t, std::string>* shared_libs, bool use_sparse_entries,
                    bool collapse_key_stringpool,
                    const std::set<ResourceName>& name_collapse_exemptions)
@@ -243,20 +248,20 @@ class PackageFlattener {
     TRACE_CALL();
     ChunkWriter pkg_writer(buffer);
     ResTable_package* pkg_header = pkg_writer.StartChunk<ResTable_package>(RES_TABLE_PACKAGE_TYPE);
-    pkg_header->id = util::HostToDevice32(package_->id.value());
+    pkg_header->id = util::HostToDevice32(package_.id.value());
 
     // AAPT truncated the package name, so do the same.
     // Shared libraries require full package names, so don't truncate theirs.
     if (context_->GetPackageType() != PackageType::kApp &&
-        package_->name.size() >= arraysize(pkg_header->name)) {
+        package_.name.size() >= arraysize(pkg_header->name)) {
-      diag_->Error(DiagMessage() << "package name '" << package_->name
+      diag_->Error(DiagMessage() << "package name '" << package_.name
                                  << "' is too long. "
                                     "Shared libraries cannot have truncated package names");
       return false;
     }
 
     // Copy the package name in device endianness.
-    strcpy16_htod(pkg_header->name, arraysize(pkg_header->name), util::Utf8ToUtf16(package_->name));
+    strcpy16_htod(pkg_header->name, arraysize(pkg_header->name), util::Utf8ToUtf16(package_.name));
 
     // Serialize the types. We do this now so that our type and key strings
     // are populated. We write those first.
@@ -273,7 +278,7 @@ class PackageFlattener {
     buffer->AppendBuffer(std::move(type_buffer));
 
     // If there are libraries (or if the package ID is 0x00), encode a library chunk.
-    if (package_->id.value() == 0x00 || !shared_libs_->empty()) {
+    if (package_.id.value() == 0x00 || !shared_libs_->empty()) {
       FlattenLibrarySpec(buffer);
     }
 
@@ -315,7 +320,7 @@ class PackageFlattener {
   }
 
   bool FlattenValue(FlatEntry* entry, BigBuffer* buffer) {
-    if (Item* item = ValueCast<Item>(entry->value)) {
+    if (const Item* item = ValueCast<Item>(entry->value)) {
       WriteEntry<ResTable_entry, true>(entry, buffer);
       Res_value* outValue = buffer->NextBlock<Res_value>();
       CHECK(item->Flatten(outValue)) << "flatten failed";
@@ -329,7 +334,7 @@ class PackageFlattener {
     return true;
   }
 
-  bool FlattenConfig(const ResourceTableType* type, const ConfigDescription& config,
+  bool FlattenConfig(const ResourceTableTypeView& type, const ConfigDescription& config,
                      const size_t num_total_entries, std::vector<FlatEntry>* entries,
                      BigBuffer* buffer) {
     CHECK(num_total_entries != 0);
@@ -337,7 +342,7 @@ class PackageFlattener {
 
     ChunkWriter type_writer(buffer);
     ResTable_type* type_header = type_writer.StartChunk<ResTable_type>(RES_TABLE_TYPE_TYPE);
-    type_header->id = type->id.value();
+    type_header->id = type.id.value();
     type_header->config = config;
     type_header->config.swapHtoD();
 
@@ -346,12 +351,12 @@ class PackageFlattener {
 
     BigBuffer values_buffer(512);
     for (FlatEntry& flat_entry : *entries) {
-      CHECK(static_cast<size_t>(flat_entry.entry->id.value()) < num_total_entries);
+      CHECK(static_cast<size_t>(flat_entry.entry->id.value().entry_id()) < num_total_entries);
-      offsets[flat_entry.entry->id.value()] = values_buffer.size();
+      offsets[flat_entry.entry->id.value().entry_id()] = values_buffer.size();
       if (!FlattenValue(&flat_entry, &values_buffer)) {
         diag_->Error(DiagMessage()
                      << "failed to flatten resource '"
-                     << ResourceNameRef(package_->name, type->type, flat_entry.entry->name)
+                     << ResourceNameRef(package_.name, type.type, flat_entry.entry->name)
                      << "' for configuration '" << config << "'");
         return false;
       }
@@ -399,55 +404,27 @@ class PackageFlattener {
     return true;
   }
 
-  std::vector<ResourceTableType*> CollectAndSortTypes() {
-    std::vector<ResourceTableType*> sorted_types;
-    for (auto& type : package_->types) {
-      if (type->type == ResourceType::kStyleable) {
-        // Styleables aren't real Resource Types, they are represented in the
-        // R.java file.
-        continue;
-      }
-
-      CHECK(bool(type->id)) << "type must have an ID set";
-
-      sorted_types.push_back(type.get());
-    }
-    std::sort(sorted_types.begin(), sorted_types.end(), cmp_ids<ResourceTableType>);
-    return sorted_types;
-  }
-
-  std::vector<ResourceEntry*> CollectAndSortEntries(ResourceTableType* type) {
-    // Sort the entries by entry ID.
-    std::vector<ResourceEntry*> sorted_entries;
-    for (auto& entry : type->entries) {
-      CHECK(bool(entry->id)) << "entry must have an ID set";
-      sorted_entries.push_back(entry.get());
-    }
-    std::sort(sorted_entries.begin(), sorted_entries.end(), cmp_ids<ResourceEntry>);
-    return sorted_entries;
-  }
-
   bool FlattenOverlayable(BigBuffer* buffer) {
     std::set<ResourceId> seen_ids;
     std::map<std::string, OverlayableChunk> overlayable_chunks;
 
-    CHECK(bool(package_->id)) << "package must have an ID set when flattening <overlayable>";
+    CHECK(bool(package_.id)) << "package must have an ID set when flattening <overlayable>";
-    for (auto& type : package_->types) {
+    for (auto& type : package_.types) {
-      CHECK(bool(type->id)) << "type must have an ID set when flattening <overlayable>";
+      CHECK(bool(type.id)) << "type must have an ID set when flattening <overlayable>";
-      for (auto& entry : type->entries) {
+      for (auto& entry : type.entries) {
-        CHECK(bool(type->id)) << "entry must have an ID set when flattening <overlayable>";
+        CHECK(bool(type.id)) << "entry must have an ID set when flattening <overlayable>";
         if (!entry->overlayable_item) {
           continue;
         }
 
-        OverlayableItem& item = entry->overlayable_item.value();
+        const OverlayableItem& item = entry->overlayable_item.value();
 
         // Resource ids should only appear once in the resource table
-        ResourceId id = android::make_resid(package_->id.value(), type->id.value(),
+        ResourceId id =
-                                            entry->id.value());
+            android::make_resid(package_.id.value(), type.id.value(), entry->id.value().entry_id());
         CHECK(seen_ids.find(id) == seen_ids.end())
             << "multiple overlayable definitions found for resource "
-            << ResourceName(package_->name, type->type, entry->name).to_string();
+            << ResourceName(package_.name, type.type, entry->name).to_string();
         seen_ids.insert(id);
 
         // Find the overlayable chunk with the specified name
@@ -542,14 +519,14 @@ class PackageFlattener {
     return true;
   }
 
-  bool FlattenTypeSpec(ResourceTableType* type, std::vector<ResourceEntry*>* sorted_entries,
+  bool FlattenTypeSpec(const ResourceTableTypeView& type,
-                       BigBuffer* buffer) {
+                       const std::vector<const ResourceEntry*>& sorted_entries, BigBuffer* buffer) {
     ChunkWriter type_spec_writer(buffer);
     ResTable_typeSpec* spec_header =
         type_spec_writer.StartChunk<ResTable_typeSpec>(RES_TABLE_TYPE_SPEC_TYPE);
-    spec_header->id = type->id.value();
+    spec_header->id = type.id.value();
 
-    if (sorted_entries->empty()) {
+    if (sorted_entries.empty()) {
       type_spec_writer.Finish();
       return true;
     }
@@ -557,7 +534,7 @@ class PackageFlattener {
     // We can't just take the size of the vector. There may be holes in the
     // entry ID space.
     // Since the entries are sorted by ID, the last one will be the biggest.
-    const size_t num_entries = sorted_entries->back()->id.value() + 1;
+    const size_t num_entries = sorted_entries.back()->id.value().entry_id() + 1;
 
     spec_header->entryCount = util::HostToDevice32(num_entries);
 
@@ -565,21 +542,19 @@ class PackageFlattener {
     // show for which configuration axis the resource changes.
     uint32_t* config_masks = type_spec_writer.NextBlock<uint32_t>(num_entries);
 
-    const size_t actual_num_entries = sorted_entries->size();
+    for (const ResourceEntry* entry : sorted_entries) {
-    for (size_t entryIndex = 0; entryIndex < actual_num_entries; entryIndex++) {
+      const uint16_t entry_id = entry->id.value().entry_id();
-      ResourceEntry* entry = sorted_entries->at(entryIndex);
 
       // Populate the config masks for this entry.
       if (entry->visibility.level == Visibility::Level::kPublic) {
-        config_masks[entry->id.value()] |= util::HostToDevice32(ResTable_typeSpec::SPEC_PUBLIC);
+        config_masks[entry_id] |= util::HostToDevice32(ResTable_typeSpec::SPEC_PUBLIC);
       }
 
       const size_t config_count = entry->values.size();
       for (size_t i = 0; i < config_count; i++) {
         const ConfigDescription& config = entry->values[i]->config;
         for (size_t j = i + 1; j < config_count; j++) {
-          config_masks[entry->id.value()] |=
+          config_masks[entry_id] |= util::HostToDevice32(config.diff(entry->values[j]->config));
-              util::HostToDevice32(config.diff(entry->values[j]->config));
         }
       }
     }
@@ -590,32 +565,31 @@ class PackageFlattener {
   bool FlattenTypes(BigBuffer* buffer) {
     // Sort the types by their IDs. They will be inserted into the StringPool in
     // this order.
-    std::vector<ResourceTableType*> sorted_types = CollectAndSortTypes();
 
     size_t expected_type_id = 1;
-    for (ResourceTableType* type : sorted_types) {
+    for (const ResourceTableTypeView& type : package_.types) {
+      if (type.type == ResourceType::kStyleable) {
+        // Styleables aren't real Resource Types, they are represented in the R.java file.
+        continue;
+      }
+
       // If there is a gap in the type IDs, fill in the StringPool
       // with empty values until we reach the ID we expect.
-      while (type->id.value() > expected_type_id) {
+      while (type.id.value() > expected_type_id) {
         std::stringstream type_name;
         type_name << "?" << expected_type_id;
         type_pool_.MakeRef(type_name.str());
         expected_type_id++;
       }
       expected_type_id++;
-      type_pool_.MakeRef(to_string(type->type));
+      type_pool_.MakeRef(to_string(type.type));
 
-      std::vector<ResourceEntry*> sorted_entries = CollectAndSortEntries(type);
+      if (!FlattenTypeSpec(type, type.entries, buffer)) {
-      if (sorted_entries.empty()) {
-        continue;
-      }
-
-      if (!FlattenTypeSpec(type, &sorted_entries, buffer)) {
         return false;
       }
 
       // Since the entries are sorted by ID, the last ID will be the largest.
-      const size_t num_entries = sorted_entries.back()->id.value() + 1;
+      const size_t num_entries = type.entries.back()->id.value().entry_id() + 1;
 
       // The binary resource table lists resource entries for each
       // configuration.
@@ -628,9 +602,9 @@ class PackageFlattener {
       // hardcoded string uses characters which make it an invalid resource name
       const std::string obfuscated_resource_name = "0_resource_name_obfuscated";
 
-      for (ResourceEntry* entry : sorted_entries) {
+      for (const ResourceEntry* entry : type.entries) {
         uint32_t local_key_index;
-        ResourceName resource_name({}, type->type, entry->name);
+        ResourceName resource_name({}, type.type, entry->name);
         if (!collapse_key_stringpool_ ||
             name_collapse_exemptions_.find(resource_name) != name_collapse_exemptions_.end()) {
           local_key_index = (uint32_t)key_pool_.MakeRef(entry->name).index();
@@ -660,17 +634,17 @@ class PackageFlattener {
     ResTable_lib_header* lib_header =
         lib_writer.StartChunk<ResTable_lib_header>(RES_TABLE_LIBRARY_TYPE);
 
-    const size_t num_entries = (package_->id.value() == 0x00 ? 1 : 0) + shared_libs_->size();
+    const size_t num_entries = (package_.id.value() == 0x00 ? 1 : 0) + shared_libs_->size();
     CHECK(num_entries > 0);
 
     lib_header->count = util::HostToDevice32(num_entries);
 
     ResTable_lib_entry* lib_entry = buffer->NextBlock<ResTable_lib_entry>(num_entries);
-    if (package_->id.value() == 0x00) {
+    if (package_.id.value() == 0x00) {
       // Add this package
       lib_entry->packageId = util::HostToDevice32(0x00);
       strcpy16_htod(lib_entry->packageName, arraysize(lib_entry->packageName),
-                    util::Utf8ToUtf16(package_->name));
+                    util::Utf8ToUtf16(package_.name));
       ++lib_entry;
     }
 
@@ -685,7 +659,7 @@ class PackageFlattener {
 
   IAaptContext* context_;
   IDiagnostics* diag_;
-  ResourceTablePackage* package_;
+  const ResourceTablePackageView package_;
   const std::map<size_t, std::string>* shared_libs_;
   bool use_sparse_entries_;
   StringPool type_pool_;
@@ -709,9 +683,10 @@ bool TableFlattener::Consume(IAaptContext* context, ResourceTable* table) {
   });
 
   // Write the ResTable header.
+  const auto& table_view = table->GetPartitionedView();
   ChunkWriter table_writer(buffer_);
   ResTable_header* table_header = table_writer.StartChunk<ResTable_header>(RES_TABLE_TYPE);
-  table_header->packageCount = util::HostToDevice32(table->packages.size());
+  table_header->packageCount = util::HostToDevice32(table_view.packages.size());
 
   // Flatten the values string pool.
   StringPool::FlattenUtf8(table_writer.buffer(), table->string_pool,
@@ -720,24 +695,25 @@ bool TableFlattener::Consume(IAaptContext* context, ResourceTable* table) {
   BigBuffer package_buffer(1024);
 
   // Flatten each package.
-  for (auto& package : table->packages) {
+  for (auto& package : table_view.packages) {
     if (context->GetPackageType() == PackageType::kApp) {
       // Write a self mapping entry for this package if the ID is non-standard (0x7f).
-      const uint8_t package_id = package->id.value();
+      CHECK((bool)package.id) << "Resource ids have not been assigned before flattening the table";
+      const uint8_t package_id = package.id.value();
       if (package_id != kFrameworkPackageId && package_id != kAppPackageId) {
-        auto result = table->included_packages_.insert({package_id, package->name});
+        auto result = table->included_packages_.insert({package_id, package.name});
-        if (!result.second && result.first->second != package->name) {
+        if (!result.second && result.first->second != package.name) {
           // A mapping for this package ID already exists, and is a different package. Error!
           context->GetDiagnostics()->Error(
               DiagMessage() << android::base::StringPrintf(
                   "can't map package ID %02x to '%s'. Already mapped to '%s'", package_id,
-                  package->name.c_str(), result.first->second.c_str()));
+                  package.name.c_str(), result.first->second.c_str()));
           return false;
         }
       }
     }
 
-    PackageFlattener flattener(context, package.get(), &table->included_packages_,
+    PackageFlattener flattener(context, package, &table->included_packages_,
                                options_.use_sparse_entries, options_.collapse_key_stringpool,
                                options_.name_collapse_exemptions);
     if (!flattener.FlattenPackage(&package_buffer)) {

tools/aapt2/format/binary/TableFlattener_test.cpp

@@ -155,7 +155,6 @@ class TableFlattenerTest : public ::testing::Test {
 TEST_F(TableFlattenerTest, FlattenFullyLinkedTable) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddSimple("com.app.test:id/one", ResourceId(0x7f020000))
           .AddSimple("com.app.test:id/two", ResourceId(0x7f020001))
           .AddValue("com.app.test:id/three", ResourceId(0x7f020002),
@@ -204,7 +203,6 @@ TEST_F(TableFlattenerTest, FlattenFullyLinkedTable) {
 TEST_F(TableFlattenerTest, FlattenEntriesWithGapsInIds) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddSimple("com.app.test:id/one", ResourceId(0x7f020001))
           .AddSimple("com.app.test:id/three", ResourceId(0x7f020003))
           .Build();
@@ -225,7 +223,6 @@ TEST_F(TableFlattenerTest, FlattenMinMaxAttributes) {
   attr.max_int = 23;
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddValue("android:attr/foo", ResourceId(0x01010000), util::make_unique<Attribute>(attr))
           .Build();
 
@@ -248,7 +245,6 @@ TEST_F(TableFlattenerTest, FlattenArray) {
                                                                2u));
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddValue("android:array/foo", ResourceId(0x01010000), std::move(array))
           .Build();
 
@@ -300,7 +296,6 @@ static std::unique_ptr<ResourceTable> BuildTableWithSparseEntries(
     IAaptContext* context, const ConfigDescription& sparse_config, float load) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId(context->GetCompilationPackage(), context->GetPackageId())
           .Build();
 
   // Add regular entries.
@@ -311,14 +306,19 @@ static std::unique_ptr<ResourceTable> BuildTableWithSparseEntries(
     const ResourceId resid(context->GetPackageId(), 0x02, static_cast<uint16_t>(i));
     const auto value =
         util::make_unique<BinaryPrimitive>(Res_value::TYPE_INT_DEC, static_cast<uint32_t>(i));
-    CHECK(table->AddResourceWithId(name, resid, ConfigDescription::DefaultConfig(), "",
+    CHECK(table->AddResource(NewResourceBuilder(name)
-                                   std::unique_ptr<Value>(value->Clone(nullptr)),
+                                 .SetId(resid)
+                                 .SetValue(std::unique_ptr<Value>(value->Clone(nullptr)))
+                                 .Build(),
                              context->GetDiagnostics()));
 
     // Every few entries, write out a sparse_config value. This will give us the desired load.
     if (i % stride == 0) {
-      CHECK(table->AddResourceWithId(name, resid, sparse_config, "",
+      CHECK(table->AddResource(
-                                     std::unique_ptr<Value>(value->Clone(nullptr)),
+          NewResourceBuilder(name)
+              .SetId(resid)
+              .SetValue(std::unique_ptr<Value>(value->Clone(nullptr)), sparse_config)
+              .Build(),
           context->GetDiagnostics()));
     }
   }
@@ -417,7 +417,6 @@ TEST_F(TableFlattenerTest, FlattenSharedLibrary) {
       test::ContextBuilder().SetCompilationPackage("lib").SetPackageId(0x00).Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("lib", 0x00)
           .AddValue("lib:id/foo", ResourceId(0x00010000), util::make_unique<Id>())
           .Build();
   ResourceTable result;
@@ -426,7 +425,7 @@ TEST_F(TableFlattenerTest, FlattenSharedLibrary) {
   Maybe<ResourceTable::SearchResult> search_result =
       result.FindResource(test::ParseNameOrDie("lib:id/foo"));
   ASSERT_TRUE(search_result);
-  EXPECT_EQ(0x00u, search_result.value().package->id.value());
+  EXPECT_EQ(0x00u, search_result.value().entry->id.value().package_id());
 
   auto iter = result.included_packages_.find(0x00);
   ASSERT_NE(result.included_packages_.end(), iter);
@@ -438,7 +437,6 @@ TEST_F(TableFlattenerTest, FlattenSharedLibraryWithStyle) {
       test::ContextBuilder().SetCompilationPackage("lib").SetPackageId(0x00).Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("lib", 0x00)
           .AddValue("lib:style/Theme",
                     ResourceId(0x00030001),
                     test::StyleBuilder()
@@ -458,9 +456,7 @@ TEST_F(TableFlattenerTest, FlattenSharedLibraryWithStyle) {
   Maybe<ResourceTable::SearchResult> search_result =
       result.FindResource(test::ParseNameOrDie("lib:style/Theme"));
   ASSERT_TRUE(search_result);
-  EXPECT_EQ(0x00u, search_result.value().package->id.value());
+  EXPECT_EQ(0x00030001u, search_result.value().entry->id.value());
-  EXPECT_EQ(0x03u, search_result.value().type->id.value());
-  EXPECT_EQ(0x01u, search_result.value().entry->id.value());
   ASSERT_EQ(1u, search_result.value().entry->values.size());
   Value* value = search_result.value().entry->values[0]->value.get();
   Style* style = ValueCast<Style>(value);
@@ -479,7 +475,6 @@ TEST_F(TableFlattenerTest, FlattenTableReferencingSharedLibraries) {
       test::ContextBuilder().SetCompilationPackage("app").SetPackageId(0x7f).Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("app", 0x7f)
           .AddValue("app:id/foo", ResourceId(0x7f010000),
                     test::BuildReference("lib_one:id/foo", ResourceId(0x02010000)))
           .AddValue("app:id/bar", ResourceId(0x7f010001),
@@ -509,7 +504,6 @@ TEST_F(TableFlattenerTest, PackageWithNonStandardIdHasDynamicRefTable) {
   std::unique_ptr<IAaptContext> context =
       test::ContextBuilder().SetCompilationPackage("app").SetPackageId(0x80).Build();
   std::unique_ptr<ResourceTable> table = test::ResourceTableBuilder()
-                                             .SetPackageId("app", 0x80)
                                              .AddSimple("app:id/foo", ResourceId(0x80010000))
                                              .Build();
 
@@ -532,7 +526,6 @@ TEST_F(TableFlattenerTest, LongPackageNameIsTruncated) {
       test::ContextBuilder().SetCompilationPackage(kPackageName).SetPackageId(0x7f).Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId(kPackageName, 0x7f)
           .AddSimple(kPackageName + ":id/foo", ResourceId(0x7f010000))
           .Build();
 
@@ -553,7 +546,6 @@ TEST_F(TableFlattenerTest, LongSharedLibraryPackageNameIsIllegal) {
                                               .Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId(kPackageName, 0x7f)
           .AddSimple(kPackageName + ":id/foo", ResourceId(0x7f010000))
           .Build();
 
@@ -564,7 +556,6 @@ TEST_F(TableFlattenerTest, LongSharedLibraryPackageNameIsIllegal) {
 TEST_F(TableFlattenerTest, ObfuscatingResourceNamesNoNameCollapseExemptionsSucceeds) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddSimple("com.app.test:id/one", ResourceId(0x7f020000))
           .AddSimple("com.app.test:id/two", ResourceId(0x7f020001))
           .AddValue("com.app.test:id/three", ResourceId(0x7f020002),
@@ -618,7 +609,6 @@ TEST_F(TableFlattenerTest, ObfuscatingResourceNamesNoNameCollapseExemptionsSucce
 TEST_F(TableFlattenerTest, ObfuscatingResourceNamesWithNameCollapseExemptionsSucceeds) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddSimple("com.app.test:id/one", ResourceId(0x7f020000))
           .AddSimple("com.app.test:id/two", ResourceId(0x7f020001))
           .AddValue("com.app.test:id/three", ResourceId(0x7f020002),
@@ -680,7 +670,6 @@ TEST_F(TableFlattenerTest, FlattenOverlayable) {
   std::string name = "com.app.test:integer/overlayable";
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddSimple(name, ResourceId(0x7f020000))
           .SetOverlayable(name, overlayable_item)
           .Build();
@@ -717,7 +706,6 @@ TEST_F(TableFlattenerTest, FlattenMultipleOverlayablePolicies) {
 
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddSimple(name_zero, ResourceId(0x7f020000))
           .SetOverlayable(name_zero, overlayable_item_zero)
           .AddSimple(name_one, ResourceId(0x7f020001))
@@ -780,7 +768,6 @@ TEST_F(TableFlattenerTest, FlattenMultipleOverlayable) {
 
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddSimple(name_zero, ResourceId(0x7f020000))
           .SetOverlayable(name_zero, overlayable_item_zero)
           .AddSimple(name_one, ResourceId(0x7f020001))
@@ -842,7 +829,6 @@ TEST_F(TableFlattenerTest, FlattenOverlayableNoPolicyFails) {
 
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddSimple(name_zero, ResourceId(0x7f020000))
           .SetOverlayable(name_zero, overlayable_item_zero)
           .Build();

tools/aapt2/format/proto/ProtoDeserialize.cpp

@@ -425,15 +425,9 @@ static bool DeserializePackageFromPb(const pb::Package& pb_package, const ResStr
                                      io::IFileCollection* files,
                                      const std::vector<std::shared_ptr<Overlayable>>& overlayables,
                                      ResourceTable* out_table, std::string* out_error) {
-  Maybe<uint8_t> id;
-  if (pb_package.has_package_id()) {
-    id = static_cast<uint8_t>(pb_package.package_id().id());
-  }
-
   std::map<ResourceId, ResourceNameRef> id_index;
 
-  ResourceTablePackage* pkg =
+  ResourceTablePackage* pkg = out_table->FindOrCreatePackage(pb_package.package_name());
-      out_table->CreatePackageAllowingDuplicateNames(pb_package.package_name(), id);
   for (const pb::Type& pb_type : pb_package.type()) {
     const ResourceType* res_type = ParseResourceType(pb_type.name());
     if (res_type == nullptr) {
@@ -444,17 +438,15 @@ static bool DeserializePackageFromPb(const pb::Package& pb_package, const ResStr
     }
 
     ResourceTableType* type = pkg->FindOrCreateType(*res_type);
-    if (pb_type.has_type_id()) {
-      type->id = static_cast<uint8_t>(pb_type.type_id().id());
-    }
 
     for (const pb::Entry& pb_entry : pb_type.entry()) {
-      ResourceEntry* entry;
+      ResourceEntry* entry = type->CreateEntry(pb_entry.name());
-      if (pb_entry.has_entry_id()) {
+      const ResourceId resource_id(
-        auto entry_id = static_cast<uint16_t>(pb_entry.entry_id().id());
+          pb_package.has_package_id() ? static_cast<uint8_t>(pb_package.package_id().id()) : 0u,
-        entry = type->FindOrCreateEntry(pb_entry.name(), entry_id);
+          pb_type.has_type_id() ? static_cast<uint8_t>(pb_type.type_id().id()) : 0u,
-      } else {
+          pb_entry.has_entry_id() ? static_cast<uint16_t>(pb_entry.entry_id().id()) : 0u);
-        entry = type->FindOrCreateEntry(pb_entry.name());
+      if (resource_id.id != 0u) {
+        entry->id = resource_id;
       }
 
       // Deserialize the symbol status (public/private with source and comments).

tools/aapt2/format/proto/ProtoSerialize.cpp

@@ -345,28 +345,29 @@ void SerializeTableToPb(const ResourceTable& table, pb::ResourceTable* out_table
   pb_fingerprint->set_version(util::GetToolFingerprint());
 
   std::vector<Overlayable*> overlayables;
-  for (const std::unique_ptr<ResourceTablePackage>& package : table.packages) {
+  auto table_view = table.GetPartitionedView();
+  for (const auto& package : table_view.packages) {
     pb::Package* pb_package = out_table->add_package();
-    if (package->id) {
+    if (package.id) {
-      pb_package->mutable_package_id()->set_id(package->id.value());
+      pb_package->mutable_package_id()->set_id(package.id.value());
     }
-    pb_package->set_package_name(package->name);
+    pb_package->set_package_name(package.name);
 
-    for (const std::unique_ptr<ResourceTableType>& type : package->types) {
+    for (const auto& type : package.types) {
       pb::Type* pb_type = pb_package->add_type();
-      if (type->id) {
+      if (type.id) {
-        pb_type->mutable_type_id()->set_id(type->id.value());
+        pb_type->mutable_type_id()->set_id(type.id.value());
       }
-      pb_type->set_name(to_string(type->type).to_string());
+      pb_type->set_name(to_string(type.type).to_string());
 
       // hardcoded string uses characters which make it an invalid resource name
       static const char* obfuscated_resource_name = "0_resource_name_obfuscated";
-      for (const std::unique_ptr<ResourceEntry>& entry : type->entries) {
+      for (const auto& entry : type.entries) {
         pb::Entry* pb_entry = pb_type->add_entry();
         if (entry->id) {
-          pb_entry->mutable_entry_id()->set_id(entry->id.value());
+          pb_entry->mutable_entry_id()->set_id(entry->id.value().entry_id());
         }
-        ResourceName resource_name({}, type->type, entry->name);
+        ResourceName resource_name({}, type.type, entry->name);
         if (options.collapse_key_stringpool &&
             options.name_collapse_exemptions.find(resource_name) ==
             options.name_collapse_exemptions.end()) {

tools/aapt2/format/proto/ProtoSerialize_test.cpp

@@ -40,18 +40,20 @@ class MockFileCollection : public io::IFileCollection {
   MOCK_METHOD0(GetDirSeparator, char());
 };
 
-ResourceEntry* GetEntry(ResourceTable* table, const ResourceNameRef& res_name,
+ResourceEntry* GetEntry(ResourceTable* table, const ResourceNameRef& res_name) {
-                   uint32_t id) {
+  auto result = table->FindResource(res_name);
-  ResourceTablePackage* package = table->FindPackage(res_name.package);
+  return (result) ? result.value().entry : nullptr;
-  ResourceTableType* type = package->FindType(res_name.type);
+}
-  return  type->FindEntry(res_name.entry, id);
+
+ResourceEntry* GetEntry(ResourceTable* table, const ResourceNameRef& res_name, ResourceId id) {
+  auto result = table->FindResource(res_name, id);
+  return (result) ? result.value().entry : nullptr;
 }
 
 TEST(ProtoSerializeTest, SerializeSinglePackage) {
   std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .AddFileReference("com.app.a:layout/main", ResourceId(0x7f020000), "res/layout/main.xml")
           .AddReference("com.app.a:layout/other", ResourceId(0x7f020001), "com.app.a:layout/main")
           .AddString("com.app.a:string/text", {}, "hi")
@@ -60,10 +62,10 @@ TEST(ProtoSerializeTest, SerializeSinglePackage) {
                           true /*allow_new*/)
           .Build();
 
-  Visibility public_symbol;
+  ASSERT_TRUE(table->AddResource(NewResourceBuilder(test::ParseNameOrDie("com.app.a:layout/main"))
-  public_symbol.level = Visibility::Level::kPublic;
+                                     .SetId(0x7f020000)
-  ASSERT_TRUE(table->SetVisibilityWithId(test::ParseNameOrDie("com.app.a:layout/main"),
+                                     .SetVisibility({Visibility::Level::kPublic})
-                                         public_symbol, ResourceId(0x7f020000),
+                                     .Build(),
                                  context->GetDiagnostics()));
 
   Id* id = test::GetValue<Id>(table.get(), "com.app.a:id/foo");
@@ -72,25 +74,35 @@ TEST(ProtoSerializeTest, SerializeSinglePackage) {
   // Make a plural.
   std::unique_ptr<Plural> plural = util::make_unique<Plural>();
   plural->values[Plural::One] = util::make_unique<String>(table->string_pool.MakeRef("one"));
-  ASSERT_TRUE(table->AddResource(test::ParseNameOrDie("com.app.a:plurals/hey"), ConfigDescription{},
+  ASSERT_TRUE(table->AddResource(NewResourceBuilder(test::ParseNameOrDie("com.app.a:plurals/hey"))
-                                 {}, std::move(plural), context->GetDiagnostics()));
+                                     .SetValue(std::move(plural))
+                                     .Build(),
+                                 context->GetDiagnostics()));
 
   // Make a styled string.
   StyleString style_string;
   style_string.str = "hello";
   style_string.spans.push_back(Span{"b", 0u, 4u});
-  ASSERT_TRUE(
+  ASSERT_TRUE(table->AddResource(
-      table->AddResource(test::ParseNameOrDie("com.app.a:string/styled"), ConfigDescription{}, {},
+      NewResourceBuilder(test::ParseNameOrDie("com.app.a:string/styled"))
-                         util::make_unique<StyledString>(table->string_pool.MakeRef(style_string)),
+          .SetValue(util::make_unique<StyledString>(table->string_pool.MakeRef(style_string)))
+          .Build(),
       context->GetDiagnostics()));
 
   // Make a resource with different products.
-  ASSERT_TRUE(table->AddResource(
+  ASSERT_TRUE(
-      test::ParseNameOrDie("com.app.a:integer/one"), test::ParseConfigOrDie("land"), {},
+      table->AddResource(NewResourceBuilder(test::ParseNameOrDie("com.app.a:integer/one"))
-      test::BuildPrimitive(android::Res_value::TYPE_INT_DEC, 123u), context->GetDiagnostics()));
+                             .SetValue(test::BuildPrimitive(android::Res_value::TYPE_INT_DEC, 123u),
-  ASSERT_TRUE(table->AddResource(
+                                       test::ParseConfigOrDie("land"))
-      test::ParseNameOrDie("com.app.a:integer/one"), test::ParseConfigOrDie("land"), "tablet",
+                             .Build(),
-      test::BuildPrimitive(android::Res_value::TYPE_INT_HEX, 321u), context->GetDiagnostics()));
+                         context->GetDiagnostics()));
+
+  ASSERT_TRUE(
+      table->AddResource(NewResourceBuilder(test::ParseNameOrDie("com.app.a:integer/one"))
+                             .SetValue(test::BuildPrimitive(android::Res_value::TYPE_INT_HEX, 321u),
+                                       test::ParseConfigOrDie("land"), "tablet")
+                             .Build(),
+                         context->GetDiagnostics()));
 
   // Make a reference with both resource name and resource ID.
   // The reference should point to a resource outside of this table to test that both name and id
@@ -98,16 +110,20 @@ TEST(ProtoSerializeTest, SerializeSinglePackage) {
   Reference expected_ref;
   expected_ref.name = test::ParseNameOrDie("android:layout/main");
   expected_ref.id = ResourceId(0x01020000);
-  ASSERT_TRUE(table->AddResource(
+  ASSERT_TRUE(table->AddResource(NewResourceBuilder(test::ParseNameOrDie("com.app.a:layout/abc"))
-      test::ParseNameOrDie("com.app.a:layout/abc"), ConfigDescription::DefaultConfig(), {},
+                                     .SetValue(util::make_unique<Reference>(expected_ref))
-      util::make_unique<Reference>(expected_ref), context->GetDiagnostics()));
+                                     .Build(),
+                                 context->GetDiagnostics()));
 
   // Make an overlayable resource.
   OverlayableItem overlayable_item(std::make_shared<Overlayable>(
       "OverlayableName", "overlay://theme", Source("res/values/overlayable.xml", 40)));
   overlayable_item.source = Source("res/values/overlayable.xml", 42);
-  ASSERT_TRUE(table->SetOverlayable(test::ParseNameOrDie("com.app.a:integer/overlayable"),
+  ASSERT_TRUE(
-                                    overlayable_item, test::GetDiagnostics()));
+      table->AddResource(NewResourceBuilder(test::ParseNameOrDie("com.app.a:integer/overlayable"))
+                             .SetOverlayable(overlayable_item)
+                             .Build(),
+                         context->GetDiagnostics()));
 
   pb::ResourceTable pb_table;
   SerializeTableToPb(*table, &pb_table, context->GetDiagnostics());
@@ -680,7 +696,6 @@ TEST(ProtoSerializeTest, CollapsingResourceNamesNoNameCollapseExemptionsSucceeds
   std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddSimple("com.app.test:id/one", ResourceId(id_one_id))
           .AddSimple("com.app.test:id/two", ResourceId(id_two_id))
           .AddValue("com.app.test:id/three", ResourceId(id_three_id),
@@ -714,7 +729,7 @@ TEST(ProtoSerializeTest, CollapsingResourceNamesNoNameCollapseExemptionsSucceeds
 
   ResourceName real_id_resource(
       "com.app.test", ResourceType::kId, "one");
-  EXPECT_THAT(GetEntry(&new_table, real_id_resource, id_one_id), IsNull());
+  EXPECT_THAT(GetEntry(&new_table, real_id_resource), IsNull());
 
   ResourceName obfuscated_id_resource(
       "com.app.test", ResourceType::kId, "0_resource_name_obfuscated");
@@ -767,7 +782,6 @@ TEST(ProtoSerializeTest, ObfuscatingResourceNamesWithNameCollapseExemptionsSucce
   std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddSimple("com.app.test:id/one", ResourceId(id_one_id))
           .AddSimple("com.app.test:id/two", ResourceId(id_two_id))
           .AddValue("com.app.test:id/three", ResourceId(id_three_id),

tools/aapt2/java/JavaClassGenerator.cpp

@@ -542,8 +542,8 @@ bool JavaClassGenerator::ProcessType(const StringPiece& package_name_to_generate
 
     // Create an ID if there is one (static libraries don't need one).
     ResourceId id;
-    if (package.id && type.id && entry->id) {
+    if (entry->id) {
-      id = ResourceId(package.id.value(), type.id.value(), entry->id.value());
+      id = entry->id.value();
     }
 
     // We need to make sure we hide the fact that we are generating kAttrPrivate attributes.

tools/aapt2/java/JavaClassGenerator_test.cpp

@@ -34,7 +34,6 @@ namespace aapt {
 TEST(JavaClassGeneratorTest, FailWhenEntryIsJavaKeyword) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddSimple("android:id/class", ResourceId(0x01020000))
           .Build();
 
@@ -54,7 +53,6 @@ TEST(JavaClassGeneratorTest, FailWhenEntryIsJavaKeyword) {
 TEST(JavaClassGeneratorTest, TransformInvalidJavaIdentifierCharacter) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddSimple("android:id/hey-man", ResourceId(0x01020000))
           .AddValue("android:attr/cool.attr", ResourceId(0x01010000),
                     test::AttributeBuilder().Build())
@@ -84,7 +82,6 @@ TEST(JavaClassGeneratorTest, TransformInvalidJavaIdentifierCharacter) {
 TEST(JavaClassGeneratorTest, CorrectPackageNameIsUsed) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddSimple("android:id/one", ResourceId(0x01020000))
           .AddSimple("android:id/com.foo$two", ResourceId(0x01020001))
           .Build();
@@ -110,8 +107,6 @@ TEST(JavaClassGeneratorTest, CorrectPackageNameIsUsed) {
 TEST(JavaClassGeneratorTest, StyleableAttributesWithDifferentPackageName) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
-          .SetPackageId("app", 0x7f)
           .AddValue("app:attr/foo", ResourceId(0x7f010000),
                     test::AttributeBuilder().Build())
           .AddValue("app:attr/bar", ResourceId(0x7f010001),
@@ -159,7 +154,6 @@ TEST(JavaClassGeneratorTest, StyleableAttributesWithDifferentPackageName) {
 TEST(JavaClassGeneratorTest, AttrPrivateIsWrittenAsAttr) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddSimple("android:attr/two", ResourceId(0x01010001))
           .AddSimple("android:^attr-private/one", ResourceId(0x01010000))
           .Build();
@@ -184,7 +178,6 @@ TEST(JavaClassGeneratorTest, OnlyWritePublicResources) {
   StdErrDiagnostics diag;
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddSimple("android:id/one", ResourceId(0x01020000))
           .AddSimple("android:id/two", ResourceId(0x01020001))
           .AddSimple("android:id/three", ResourceId(0x01020002))
@@ -276,8 +269,6 @@ ResourceId>()),
 TEST(JavaClassGeneratorTest, EmitOtherPackagesAttributesInStyleable) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
-          .SetPackageId("com.lib", 0x02)
           .AddValue("android:attr/bar", ResourceId(0x01010000), test::AttributeBuilder().Build())
           .AddValue("com.lib:attr/bar", ResourceId(0x02010000), test::AttributeBuilder().Build())
           .AddValue("android:styleable/foo", ResourceId(0x01030000),
@@ -306,7 +297,6 @@ TEST(JavaClassGeneratorTest, EmitOtherPackagesAttributesInStyleable) {
 TEST(JavaClassGeneratorTest, CommentsForSimpleResourcesArePresent) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddSimple("android:id/foo", ResourceId(0x01010000))
           .Build();
   test::GetValue<Id>(table.get(), "android:id/foo")
@@ -346,7 +336,6 @@ TEST(JavaClassGeneratorTest, CommentsForStyleablesAndNestedAttributesArePresent)
 
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddValue("android:attr/one", util::make_unique<Attribute>(attr))
           .AddValue("android:styleable/Container",
                     std::unique_ptr<Styleable>(styleable.Clone(nullptr)))
@@ -384,7 +373,6 @@ TEST(JavaClassGeneratorTest, CommentsForStyleableHiddenAttributesAreNotPresent)
 
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddValue("android:attr/one", util::make_unique<Attribute>(attr))
           .AddValue("android:styleable/Container",
                     std::unique_ptr<Styleable>(styleable.Clone(nullptr)))
@@ -415,7 +403,6 @@ TEST(JavaClassGeneratorTest, CommentsForStyleableHiddenAttributesAreNotPresent)
 TEST(JavaClassGeneratorTest, StyleableAndIndicesAreColocated) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddValue("android:attr/layout_gravity", util::make_unique<Attribute>())
           .AddValue("android:attr/background", util::make_unique<Attribute>())
           .AddValue("android:styleable/ActionBar",
@@ -467,7 +454,6 @@ TEST(JavaClassGeneratorTest, CommentsForRemovedAttributesAreNotPresentInClass) {
 
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddValue("android:attr/one", util::make_unique<Attribute>(attr))
           .Build();
 
@@ -499,7 +485,6 @@ TEST(JavaClassGeneratorTest, CommentsForRemovedAttributesAreNotPresentInClass) {
 TEST(JavaClassGeneratorTest, GenerateOnResourcesLoadedCallbackForSharedLibrary) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x00)
           .AddValue("android:attr/foo", ResourceId(0x00010000), util::make_unique<Attribute>())
           .AddValue("android:id/foo", ResourceId(0x00020000), util::make_unique<Id>())
           .AddValue(
@@ -536,7 +521,6 @@ TEST(JavaClassGeneratorTest, GenerateOnResourcesLoadedCallbackForSharedLibrary)
 TEST(JavaClassGeneratorTest, OnlyGenerateRText) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddValue("android:attr/foo", ResourceId(0x01010000), util::make_unique<Attribute>())
           .AddValue("android:styleable/hey.dude", ResourceId(0x01020000),
                     test::StyleableBuilder()
@@ -554,8 +538,6 @@ TEST(JavaClassGeneratorTest, OnlyGenerateRText) {
 TEST(JavaClassGeneratorTest, SortsDynamicAttributesAfterFrameworkAttributes) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
-          .SetPackageId("lib", 0x00)
           .AddValue("android:attr/framework_attr", ResourceId(0x01010000),
                     test::AttributeBuilder().Build())
           .AddValue("lib:attr/dynamic_attr", ResourceId(0x00010000),

tools/aapt2/java/ProguardRules_test.cpp

@@ -247,8 +247,10 @@ TEST(ProguardRulesTest, IncludedLayoutRulesAreConditional) {
 
   ResourceTable table;
   StdErrDiagnostics errDiagnostics;
-  table.AddResource(bar_layout->file.name, ConfigDescription::DefaultConfig(), "",
+  table.AddResource(NewResourceBuilder(bar_layout->file.name)
-                    util::make_unique<FileReference>(), &errDiagnostics);
+                        .SetValue(util::make_unique<FileReference>())
+                        .Build(),
+                    &errDiagnostics);
 
   std::unique_ptr<IAaptContext> context =
       test::ContextBuilder()

tools/aapt2/link/AutoVersioner_test.cpp

@@ -54,7 +54,6 @@ TEST(AutoVersionerTest, GenerateVersionedResourceWhenHigherVersionExists) {
 TEST(AutoVersionerTest, VersionStylesForTable) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("app", 0x7f)
           .AddValue(
               "app:style/Foo", test::ParseConfigOrDie("v4"),
               ResourceId(0x7f020000),

tools/aapt2/link/NoDefaultResourceRemover_test.cpp

@@ -21,10 +21,9 @@
 namespace aapt {
 
 TEST(NoDefaultResourceRemoverTest, RemoveEntryWithNoDefaultAndOnlyLocales) {
-  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
+  std::unique_ptr<IAaptContext> context = test::ContextBuilder().SetPackageId(0x01).Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddSimple("android:string/foo")
           .AddSimple("android:string/foo", test::ParseConfigOrDie("en-rGB"))
           .AddSimple("android:string/foo", test::ParseConfigOrDie("fr-rFR"))
@@ -47,10 +46,10 @@ TEST(NoDefaultResourceRemoverTest, RemoveEntryWithNoDefaultAndOnlyLocales) {
 }
 
 TEST(NoDefaultResourceRemoverTest, KeepEntryWithLocalesAndDensities) {
-  std::unique_ptr<IAaptContext> context = test::ContextBuilder().SetMinSdkVersion(26).Build();
+  std::unique_ptr<IAaptContext> context =
+      test::ContextBuilder().SetPackageId(0x01).SetMinSdkVersion(26).Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddSimple("android:drawable/keep1", test::ParseConfigOrDie("mdpi")) // v4
           .AddSimple("android:drawable/keep1", test::ParseConfigOrDie("en-rGB"))
           .AddSimple("android:drawable/keep1", test::ParseConfigOrDie("fr-rFR"))
@@ -71,10 +70,10 @@ TEST(NoDefaultResourceRemoverTest, KeepEntryWithLocalesAndDensities) {
 }
 
 TEST(NoDefaultResourceRemoverTest, RemoveEntryWithLocalesAndDensitiesLowVersion) {
-  std::unique_ptr<IAaptContext> context = test::ContextBuilder().SetMinSdkVersion(3).Build();
+  std::unique_ptr<IAaptContext> context =
+      test::ContextBuilder().SetPackageId(0x01).SetMinSdkVersion(3).Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddSimple("android:drawable/remove1", test::ParseConfigOrDie("mdpi")) // v4
           .AddSimple("android:drawable/remove1", test::ParseConfigOrDie("en-rGB"))
           .AddSimple("android:drawable/remove1", test::ParseConfigOrDie("fr-rFR"))
@@ -87,10 +86,10 @@ TEST(NoDefaultResourceRemoverTest, RemoveEntryWithLocalesAndDensitiesLowVersion)
 }
 
 TEST(NoDefaultResourceRemoverTest, KeepEntryWithVersion) {
-  std::unique_ptr<IAaptContext> context = test::ContextBuilder().SetMinSdkVersion(8).Build();
+  std::unique_ptr<IAaptContext> context =
+      test::ContextBuilder().SetPackageId(0x01).SetMinSdkVersion(8).Build();
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("android", 0x01)
           .AddSimple("android:drawable/keep1", test::ParseConfigOrDie("v8"))
           .AddSimple("android:drawable/keep1", test::ParseConfigOrDie("en-rGB"))
           .AddSimple("android:drawable/keep1", test::ParseConfigOrDie("fr-rFR"))

tools/aapt2/link/ProductFilter_test.cpp

@@ -30,21 +30,29 @@ TEST(ProductFilterTest, SelectTwoProducts) {
 
   ResourceTable table;
   ASSERT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:string/one"), land, "",
+      NewResourceBuilder(test::ParseNameOrDie("android:string/one"))
-      test::ValueBuilder<Id>().SetSource(Source("land/default.xml")).Build(),
+          .SetValue(test::ValueBuilder<Id>().SetSource(Source("land/default.xml")).Build(), land)
-      context->GetDiagnostics()));
+          .Build(),
-  ASSERT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:string/one"), land, "tablet",
-      test::ValueBuilder<Id>().SetSource(Source("land/tablet.xml")).Build(),
       context->GetDiagnostics()));
 
   ASSERT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:string/one"), port, "",
+      NewResourceBuilder(test::ParseNameOrDie("android:string/one"))
-      test::ValueBuilder<Id>().SetSource(Source("port/default.xml")).Build(),
+          .SetValue(test::ValueBuilder<Id>().SetSource(Source("land/tablet.xml")).Build(), land,
+                    "tablet")
+          .Build(),
       context->GetDiagnostics()));
+
   ASSERT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:string/one"), port, "tablet",
+      NewResourceBuilder(test::ParseNameOrDie("android:string/one"))
-      test::ValueBuilder<Id>().SetSource(Source("port/tablet.xml")).Build(),
+          .SetValue(test::ValueBuilder<Id>().SetSource(Source("port/default.xml")).Build(), port)
+          .Build(),
+      context->GetDiagnostics()));
+
+  ASSERT_TRUE(table.AddResource(
+      NewResourceBuilder(test::ParseNameOrDie("android:string/one"))
+          .SetValue(test::ValueBuilder<Id>().SetSource(Source("port/tablet.xml")).Build(), port,
+                    "tablet")
+          .Build(),
       context->GetDiagnostics()));
 
   ProductFilter filter({"tablet"});
@@ -65,15 +73,17 @@ TEST(ProductFilterTest, SelectDefaultProduct) {
 
   ResourceTable table;
   ASSERT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:string/one"),
+      NewResourceBuilder(test::ParseNameOrDie("android:string/one"))
-      ConfigDescription::DefaultConfig(), "",
+          .SetValue(test::ValueBuilder<Id>().SetSource(Source("default.xml")).Build())
-      test::ValueBuilder<Id>().SetSource(Source("default.xml")).Build(),
+          .Build(),
       context->GetDiagnostics()));
+
   ASSERT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:string/one"),
+      NewResourceBuilder(test::ParseNameOrDie("android:string/one"))
-      ConfigDescription::DefaultConfig(), "tablet",
+          .SetValue(test::ValueBuilder<Id>().SetSource(Source("tablet.xml")).Build(), {}, "tablet")
-      test::ValueBuilder<Id>().SetSource(Source("tablet.xml")).Build(),
+          .Build(),
       context->GetDiagnostics()));
+  ;
 
   ProductFilter filter(std::unordered_set<std::string>{});
   ASSERT_TRUE(filter.Consume(context.get(), &table));
@@ -91,19 +101,22 @@ TEST(ProductFilterTest, FailOnAmbiguousProduct) {
 
   ResourceTable table;
   ASSERT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:string/one"),
+      NewResourceBuilder(test::ParseNameOrDie("android:string/one"))
-      ConfigDescription::DefaultConfig(), "",
+          .SetValue(test::ValueBuilder<Id>().SetSource(Source("default.xml")).Build())
-      test::ValueBuilder<Id>().SetSource(Source("default.xml")).Build(),
+          .Build(),
       context->GetDiagnostics()));
+
   ASSERT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:string/one"),
+      NewResourceBuilder(test::ParseNameOrDie("android:string/one"))
-      ConfigDescription::DefaultConfig(), "tablet",
+          .SetValue(test::ValueBuilder<Id>().SetSource(Source("tablet.xml")).Build(), {}, "tablet")
-      test::ValueBuilder<Id>().SetSource(Source("tablet.xml")).Build(),
+          .Build(),
       context->GetDiagnostics()));
+
   ASSERT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:string/one"),
+      NewResourceBuilder(test::ParseNameOrDie("android:string/one"))
-      ConfigDescription::DefaultConfig(), "no-sdcard",
+          .SetValue(test::ValueBuilder<Id>().SetSource(Source("no-sdcard.xml")).Build(), {},
-      test::ValueBuilder<Id>().SetSource(Source("no-sdcard.xml")).Build(),
+                    "no-sdcard")
+          .Build(),
       context->GetDiagnostics()));
 
   ProductFilter filter({"tablet", "no-sdcard"});
@@ -114,15 +127,17 @@ TEST(ProductFilterTest, FailOnMultipleDefaults) {
   std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
 
   ResourceTable table;
-  ASSERT_TRUE(table.AddResource(
+  ASSERT_TRUE(
-      test::ParseNameOrDie("android:string/one"),
+      table.AddResource(NewResourceBuilder(test::ParseNameOrDie("android:string/one"))
-      ConfigDescription::DefaultConfig(), "",
+                            .SetValue(test::ValueBuilder<Id>().SetSource(Source(".xml")).Build())
-      test::ValueBuilder<Id>().SetSource(Source(".xml")).Build(),
+                            .Build(),
                         context->GetDiagnostics()));
+
   ASSERT_TRUE(table.AddResource(
-      test::ParseNameOrDie("android:string/one"),
+      NewResourceBuilder(test::ParseNameOrDie("android:string/one"))
-      ConfigDescription::DefaultConfig(), "default",
+          .SetValue(test::ValueBuilder<Id>().SetSource(Source("default.xml")).Build(), {},
-      test::ValueBuilder<Id>().SetSource(Source("default.xml")).Build(),
+                    "default")
+          .Build(),
       context->GetDiagnostics()));
 
   ProductFilter filter(std::unordered_set<std::string>{});

tools/aapt2/link/ReferenceLinker_test.cpp

@@ -28,7 +28,6 @@ namespace aapt {
 TEST(ReferenceLinkerTest, LinkSimpleReferences) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddReference("com.app.test:string/foo", ResourceId(0x7f020000),
                         "com.app.test:string/bar")
 
@@ -75,7 +74,6 @@ TEST(ReferenceLinkerTest, LinkSimpleReferences) {
 TEST(ReferenceLinkerTest, LinkStyleAttributes) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddValue("com.app.test:style/Theme",
                     test::StyleBuilder()
                         .SetParent("android:style/Theme.Material")
@@ -155,7 +153,6 @@ TEST(ReferenceLinkerTest, LinkMangledReferencesAndAttributes) {
 
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddValue("com.app.test:style/Theme", ResourceId(0x7f020000),
                     test::StyleBuilder()
                         .AddItem("com.android.support:attr/foo",
@@ -176,7 +173,6 @@ TEST(ReferenceLinkerTest, LinkMangledReferencesAndAttributes) {
 TEST(ReferenceLinkerTest, FailToLinkPrivateSymbols) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddReference("com.app.test:string/foo", ResourceId(0x7f020000),
                         "android:string/hidden")
           .Build();
@@ -201,7 +197,6 @@ TEST(ReferenceLinkerTest, FailToLinkPrivateSymbols) {
 TEST(ReferenceLinkerTest, FailToLinkPrivateMangledSymbols) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddReference("com.app.test:string/foo", ResourceId(0x7f020000),
                         "com.app.lib:string/hidden")
           .Build();
@@ -229,7 +224,6 @@ TEST(ReferenceLinkerTest, FailToLinkPrivateMangledSymbols) {
 TEST(ReferenceLinkerTest, FailToLinkPrivateStyleAttributes) {
   std::unique_ptr<ResourceTable> table =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.test", 0x7f)
           .AddValue("com.app.test:style/Theme",
                     test::StyleBuilder()
                         .AddItem("android:attr/hidden",

tools/aapt2/link/TableMerger.cpp

@@ -33,8 +33,7 @@ TableMerger::TableMerger(IAaptContext* context, ResourceTable* out_table,
                          const TableMergerOptions& options)
     : context_(context), main_table_(out_table), options_(options) {
   // Create the desired package that all tables will be merged into.
-  main_package_ =
+  main_package_ = main_table_->FindOrCreatePackage(context_->GetCompilationPackage());
-      main_table_->CreatePackage(context_->GetCompilationPackage(), context_->GetPackageId());
   CHECK(main_package_ != nullptr) << "package name or ID already taken";
 }
 
@@ -85,20 +84,9 @@ bool TableMerger::MergeAndMangle(const Source& src, const StringPiece& package_n
 
 static bool MergeType(IAaptContext* context, const Source& src, ResourceTableType* dst_type,
                       ResourceTableType* src_type) {
-  if (src_type->visibility_level > dst_type->visibility_level) {
+  if (src_type->visibility_level >= dst_type->visibility_level) {
     // The incoming type's visibility is stronger, so we should override the visibility.
-    if (src_type->visibility_level == Visibility::Level::kPublic) {
-      // Only copy the ID if the source is public, or else the ID is meaningless.
-      dst_type->id = src_type->id;
-    }
     dst_type->visibility_level = src_type->visibility_level;
-  } else if (dst_type->visibility_level == Visibility::Level::kPublic &&
-             src_type->visibility_level == Visibility::Level::kPublic && dst_type->id &&
-             src_type->id && dst_type->id.value() != src_type->id.value()) {
-    // Both types are public and have different IDs.
-    context->GetDiagnostics()->Error(DiagMessage(src) << "cannot merge type '" << src_type->type
-                                                      << "': conflicting public IDs");
-    return false;
   }
   return true;
 }
@@ -347,7 +335,7 @@ bool TableMerger::MergeFile(const ResourceFile& file_desc, bool overlay, io::IFi
   file_ref->type = file_desc.type;
   file_ref->file = file;
 
-  ResourceTablePackage* pkg = table.CreatePackage(file_desc.name.package, 0x0);
+  ResourceTablePackage* pkg = table.FindOrCreatePackage(file_desc.name.package);
   pkg->FindOrCreateType(file_desc.name.type)
       ->FindOrCreateEntry(file_desc.name.entry)
       ->FindOrCreateValue(file_desc.config, {})

tools/aapt2/link/TableMerger_test.cpp

@@ -55,16 +55,13 @@ struct TableMergerTest : public ::testing::Test {
 TEST_F(TableMergerTest, SimpleMerge) {
   std::unique_ptr<ResourceTable> table_a =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .AddReference("com.app.a:id/foo", "com.app.a:id/bar")
           .AddReference("com.app.a:id/bar", "com.app.b:id/foo")
-          .AddValue(
+          .AddValue("com.app.a:styleable/view",
-              "com.app.a:styleable/view",
                     test::StyleableBuilder().AddItem("com.app.b:id/foo").Build())
           .Build();
 
   std::unique_ptr<ResourceTable> table_b = test::ResourceTableBuilder()
-                                               .SetPackageId("com.app.b", 0x7f)
                                                .AddSimple("com.app.b:id/foo")
                                                .Build();
 
@@ -129,12 +126,10 @@ TEST_F(TableMergerTest, MergeFileReferences) {
 
   std::unique_ptr<ResourceTable> table_a =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .AddFileReference("com.app.a:xml/file", "res/xml/file.xml", &file_a)
           .Build();
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.b", 0x7f)
           .AddFileReference("com.app.b:xml/file", "res/xml/file.xml", &file_b)
           .Build();
 
@@ -158,12 +153,10 @@ TEST_F(TableMergerTest, MergeFileReferences) {
 TEST_F(TableMergerTest, OverrideResourceWithOverlay) {
   std::unique_ptr<ResourceTable> base =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x00)
           .AddValue("bool/foo", ResourceUtils::TryParseBool("true"))
           .Build();
   std::unique_ptr<ResourceTable> overlay =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x00)
           .AddValue("bool/foo", ResourceUtils::TryParseBool("false"))
           .Build();
 
@@ -194,14 +187,12 @@ TEST_F(TableMergerTest, DoNotOverrideResourceComment) {
 
   std::unique_ptr<ResourceTable> base =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x00)
           .AddValue("bool/foo", std::move(foo_original))
           .AddValue("bool/bar", std::move(bar_original))
           .Build();
 
   std::unique_ptr<ResourceTable> overlay =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x00)
           .AddValue("bool/foo", std::move(foo_overlay))
           .AddValue("bool/bar", std::move(bar_overlay))
           .AddValue("bool/baz", std::move(baz_overlay))
@@ -226,12 +217,10 @@ TEST_F(TableMergerTest, DoNotOverrideResourceComment) {
 TEST_F(TableMergerTest, OverrideSameResourceIdsWithOverlay) {
   std::unique_ptr<ResourceTable> base =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x7f)
           .SetSymbolState("bool/foo", ResourceId(0x7f, 0x01, 0x0001), Visibility::Level::kPublic)
           .Build();
   std::unique_ptr<ResourceTable> overlay =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x7f)
           .SetSymbolState("bool/foo", ResourceId(0x7f, 0x01, 0x0001), Visibility::Level::kPublic)
           .Build();
 
@@ -247,12 +236,10 @@ TEST_F(TableMergerTest, OverrideSameResourceIdsWithOverlay) {
 TEST_F(TableMergerTest, FailToOverrideConflictingTypeIdsWithOverlay) {
   std::unique_ptr<ResourceTable> base =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x7f)
           .SetSymbolState("bool/foo", ResourceId(0x7f, 0x01, 0x0001), Visibility::Level::kPublic)
           .Build();
   std::unique_ptr<ResourceTable> overlay =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x7f)
           .SetSymbolState("bool/foo", ResourceId(0x7f, 0x02, 0x0001), Visibility::Level::kPublic)
           .Build();
 
@@ -268,12 +255,10 @@ TEST_F(TableMergerTest, FailToOverrideConflictingTypeIdsWithOverlay) {
 TEST_F(TableMergerTest, FailToOverrideConflictingEntryIdsWithOverlay) {
   std::unique_ptr<ResourceTable> base =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x7f)
           .SetSymbolState("bool/foo", ResourceId(0x7f, 0x01, 0x0001), Visibility::Level::kPublic)
           .Build();
   std::unique_ptr<ResourceTable> overlay =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x7f)
           .SetSymbolState("bool/foo", ResourceId(0x7f, 0x01, 0x0002), Visibility::Level::kPublic)
           .Build();
 
@@ -289,12 +274,10 @@ TEST_F(TableMergerTest, FailToOverrideConflictingEntryIdsWithOverlay) {
 TEST_F(TableMergerTest, FailConflictingVisibility) {
   std::unique_ptr<ResourceTable> base =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x7f)
           .SetSymbolState("bool/foo", ResourceId(0x7f, 0x01, 0x0001), Visibility::Level::kPublic)
           .Build();
   std::unique_ptr<ResourceTable> overlay =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x7f)
           .SetSymbolState("bool/foo", ResourceId(0x7f, 0x01, 0x0001), Visibility::Level::kPrivate)
           .Build();
 
@@ -318,11 +301,9 @@ TEST_F(TableMergerTest, FailConflictingVisibility) {
 }
 
 TEST_F(TableMergerTest, MergeAddResourceFromOverlay) {
-  std::unique_ptr<ResourceTable> table_a =
+  std::unique_ptr<ResourceTable> table_a = test::ResourceTableBuilder().Build();
-      test::ResourceTableBuilder().SetPackageId("", 0x7f).Build();
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x7f)
           .SetSymbolState("bool/foo", {}, Visibility::Level::kUndefined, true /*allow new overlay*/)
           .AddValue("bool/foo", ResourceUtils::TryParseBool("true"))
           .Build();
@@ -337,11 +318,9 @@ TEST_F(TableMergerTest, MergeAddResourceFromOverlay) {
 }
 
 TEST_F(TableMergerTest, MergeAddResourceFromOverlayWithAutoAddOverlay) {
-  std::unique_ptr<ResourceTable> table_a =
+  std::unique_ptr<ResourceTable> table_a = test::ResourceTableBuilder().Build();
-      test::ResourceTableBuilder().SetPackageId("", 0x7f).Build();
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x7f)
           .AddValue("bool/foo", ResourceUtils::TryParseBool("true"))
           .Build();
 
@@ -355,11 +334,9 @@ TEST_F(TableMergerTest, MergeAddResourceFromOverlayWithAutoAddOverlay) {
 }
 
 TEST_F(TableMergerTest, FailToMergeNewResourceWithoutAutoAddOverlay) {
-  std::unique_ptr<ResourceTable> table_a =
+  std::unique_ptr<ResourceTable> table_a = test::ResourceTableBuilder().Build();
-      test::ResourceTableBuilder().SetPackageId("", 0x7f).Build();
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("", 0x7f)
           .AddValue("bool/foo", ResourceUtils::TryParseBool("true"))
           .Build();
 
@@ -375,7 +352,6 @@ TEST_F(TableMergerTest, FailToMergeNewResourceWithoutAutoAddOverlay) {
 TEST_F(TableMergerTest, OverlaidStyleablesAndStylesShouldBeMerged) {
   std::unique_ptr<ResourceTable> table_a =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .AddValue("com.app.a:styleable/Foo",
                     test::StyleableBuilder()
                         .AddItem("com.app.a:attr/bar")
@@ -391,7 +367,6 @@ TEST_F(TableMergerTest, OverlaidStyleablesAndStylesShouldBeMerged) {
 
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .AddValue("com.app.a:styleable/Foo", test::StyleableBuilder()
                                                    .AddItem("com.app.a:attr/bat")
                                                    .AddItem("com.app.a:attr/foo")
@@ -441,7 +416,6 @@ TEST_F(TableMergerTest, OverlaidStyleablesAndStylesShouldBeMerged) {
 TEST_F(TableMergerTest, OverrideStyleInsteadOfOverlaying) {
   std::unique_ptr<ResourceTable> table_a =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .AddValue(
               "com.app.a:styleable/MyWidget",
               test::StyleableBuilder().AddItem("com.app.a:attr/foo", ResourceId(0x1234)).Build())
@@ -452,7 +426,6 @@ TEST_F(TableMergerTest, OverrideStyleInsteadOfOverlaying) {
           .Build();
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .AddValue(
               "com.app.a:styleable/MyWidget",
               test::StyleableBuilder().AddItem("com.app.a:attr/bar", ResourceId(0x5678)).Build())
@@ -494,13 +467,11 @@ TEST_F(TableMergerTest, SetOverlayable) {
 
   std::unique_ptr<ResourceTable> table_a =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .SetOverlayable("bool/foo", overlayable_item)
           .Build();
 
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .AddSimple("bool/foo")
           .Build();
 
@@ -527,7 +498,6 @@ TEST_F(TableMergerTest, SetOverlayableLater) {
                                                   "overlay://customization");
   std::unique_ptr<ResourceTable> table_a =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .AddSimple("bool/foo")
           .Build();
 
@@ -536,7 +506,6 @@ TEST_F(TableMergerTest, SetOverlayableLater) {
   overlayable_item.policies |= PolicyFlags::SYSTEM_PARTITION;
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .SetOverlayable("bool/foo", overlayable_item)
           .Build();
 
@@ -565,7 +534,6 @@ TEST_F(TableMergerTest, SameResourceDifferentNameFail) {
   overlayable_item_first.policies |= PolicyFlags::PRODUCT_PARTITION;
   std::unique_ptr<ResourceTable> table_a =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .SetOverlayable("bool/foo", overlayable_item_first)
           .Build();
 
@@ -575,7 +543,6 @@ TEST_F(TableMergerTest, SameResourceDifferentNameFail) {
   overlayable_item_second.policies |= PolicyFlags::PRODUCT_PARTITION;
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .SetOverlayable("bool/foo", overlayable_item_second)
           .Build();
 
@@ -594,7 +561,6 @@ TEST_F(TableMergerTest, SameResourceDifferentActorFail) {
   overlayable_item_first.policies |= PolicyFlags::PRODUCT_PARTITION;
   std::unique_ptr<ResourceTable> table_a =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .SetOverlayable("bool/foo", overlayable_item_first)
           .Build();
 
@@ -604,7 +570,6 @@ TEST_F(TableMergerTest, SameResourceDifferentActorFail) {
   overlayable_item_second.policies |= PolicyFlags::PRODUCT_PARTITION;
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .SetOverlayable("bool/foo", overlayable_item_second)
           .Build();
 
@@ -623,7 +588,6 @@ TEST_F(TableMergerTest, SameResourceDifferentPoliciesFail) {
   overlayable_item_first.policies |= PolicyFlags::PRODUCT_PARTITION;
   std::unique_ptr<ResourceTable> table_a =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .SetOverlayable("bool/foo", overlayable_item_first)
           .Build();
 
@@ -633,7 +597,6 @@ TEST_F(TableMergerTest, SameResourceDifferentPoliciesFail) {
   overlayable_item_second.policies |= PolicyFlags::SIGNATURE;
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .SetOverlayable("bool/foo", overlayable_item_second)
           .Build();
 
@@ -653,7 +616,6 @@ TEST_F(TableMergerTest, SameResourceSameOverlayable) {
   overlayable_item_first.policies |= PolicyFlags::PRODUCT_PARTITION;
   std::unique_ptr<ResourceTable> table_a =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .SetOverlayable("bool/foo", overlayable_item_first)
           .Build();
 
@@ -661,7 +623,6 @@ TEST_F(TableMergerTest, SameResourceSameOverlayable) {
   overlayable_item_second.policies |= PolicyFlags::PRODUCT_PARTITION;
   std::unique_ptr<ResourceTable> table_b =
       test::ResourceTableBuilder()
-          .SetPackageId("com.app.a", 0x7f)
           .SetOverlayable("bool/foo", overlayable_item_second)
           .Build();
 

tools/aapt2/process/SymbolTable.cpp

@@ -197,9 +197,9 @@ std::unique_ptr<SymbolTable::Symbol> ResourceTableSymbolSource::FindByName(
   std::unique_ptr<SymbolTable::Symbol> symbol = util::make_unique<SymbolTable::Symbol>();
   symbol->is_public = (sr.entry->visibility.level == Visibility::Level::kPublic);
 
-  if (sr.package->id && sr.type->id && sr.entry->id) {
+  if (sr.entry->id) {
-    symbol->id = ResourceId(sr.package->id.value(), sr.type->id.value(), sr.entry->id.value());
+    symbol->id = sr.entry->id.value();
-    symbol->is_dynamic = (sr.package->id.value() == 0);
+    symbol->is_dynamic = (sr.entry->id.value().package_id() == 0);
   }
 
   if (name.type == ResourceType::kAttr || name.type == ResourceType::kAttrPrivate) {

tools/aapt2/split/TableSplitter.cpp

@@ -185,7 +185,7 @@ void TableSplitter::SplitTable(ResourceTable* original_table) {
     // Initialize all packages for splits.
     for (size_t idx = 0; idx < split_count; idx++) {
       ResourceTable* split_table = splits_[idx].get();
-      split_table->CreatePackage(pkg->name, pkg->id);
+      split_table->FindOrCreatePackage(pkg->name);
     }
 
     for (auto& type : pkg->types) {
@@ -241,10 +241,7 @@ void TableSplitter::SplitTable(ResourceTable* original_table) {
             // not have actual values for each type/entry.
             ResourceTablePackage* split_pkg = split_table->FindPackage(pkg->name);
             ResourceTableType* split_type = split_pkg->FindOrCreateType(type->type);
-            if (!split_type->id) {
-              split_type->id = type->id;
             split_type->visibility_level = type->visibility_level;
-            }
 
             ResourceEntry* split_entry = split_type->FindOrCreateEntry(entry->name);
             if (!split_entry->id) {

tools/aapt2/split/TableSplitter_test.cpp

@@ -193,14 +193,22 @@ TEST(TableSplitterTest, SplitTableByConfigAndDensity) {
   ResourceTable table;
 
   const ResourceName foo = test::ParseNameOrDie("android:string/foo");
-  ASSERT_TRUE(table.AddResource(foo, test::ParseConfigOrDie("land-hdpi"), {},
+  ASSERT_TRUE(
-                                util::make_unique<Id>(),
+      table.AddResource(NewResourceBuilder(foo)
+                            .SetValue(util::make_unique<Id>(), test::ParseConfigOrDie("land-hdpi"))
+                            .Build(),
                         test::GetDiagnostics()));
-  ASSERT_TRUE(table.AddResource(foo, test::ParseConfigOrDie("land-xhdpi"), {},
+
-                                util::make_unique<Id>(),
+  ASSERT_TRUE(
+      table.AddResource(NewResourceBuilder(foo)
+                            .SetValue(util::make_unique<Id>(), test::ParseConfigOrDie("land-xhdpi"))
+                            .Build(),
                         test::GetDiagnostics()));
-  ASSERT_TRUE(table.AddResource(foo, test::ParseConfigOrDie("land-xxhdpi"), {},
+
-                                util::make_unique<Id>(),
+  ASSERT_TRUE(table.AddResource(
+      NewResourceBuilder(foo)
+          .SetValue(util::make_unique<Id>(), test::ParseConfigOrDie("land-xxhdpi"))
+          .Build(),
       test::GetDiagnostics()));
 
   std::vector<SplitConstraints> constraints;

tools/aapt2/test/Builders.cpp

@@ -34,13 +34,6 @@ using ::android::StringPiece;
 namespace aapt {
 namespace test {
 
-ResourceTableBuilder& ResourceTableBuilder::SetPackageId(const StringPiece& package_name,
-                                                         uint8_t id) {
-  ResourceTablePackage* package = table_->CreatePackage(package_name, id);
-  CHECK(package != nullptr);
-  return *this;
-}
-
 ResourceTableBuilder& ResourceTableBuilder::AddSimple(const StringPiece& name,
                                                       const ResourceId& id) {
   return AddValue(name, id, util::make_unique<Id>());
@@ -118,8 +111,13 @@ ResourceTableBuilder& ResourceTableBuilder::AddValue(const StringPiece& name,
                                                      const ResourceId& id,
                                                      std::unique_ptr<Value> value) {
   ResourceName res_name = ParseNameOrDie(name);
-  CHECK(table_->AddResourceWithIdMangled(res_name, id, config, {}, std::move(value),
+  NewResourceBuilder builder(res_name);
-                                         GetDiagnostics()));
+  builder.SetValue(std::move(value), config).SetAllowMangled(true);
+  if (id.id != 0U) {
+    builder.SetId(id);
+  }
+
+  CHECK(table_->AddResource(builder.Build(), GetDiagnostics()));
   return *this;
 }
 
@@ -128,10 +126,13 @@ ResourceTableBuilder& ResourceTableBuilder::SetSymbolState(const StringPiece& na
                                                            Visibility::Level level,
                                                            bool allow_new) {
   ResourceName res_name = ParseNameOrDie(name);
-  Visibility visibility;
+  NewResourceBuilder builder(res_name);
-  visibility.level = level;
+  builder.SetVisibility({level}).SetAllowNew({}).SetAllowMangled(true);
-  CHECK(table_->SetVisibilityWithIdMangled(res_name, visibility, id, GetDiagnostics()));
+  if (id.id != 0U) {
-  CHECK(table_->SetAllowNewMangled(res_name, AllowNew{}, GetDiagnostics()));
+    builder.SetId(id);
+  }
+
+  CHECK(table_->AddResource(builder.Build(), GetDiagnostics()));
   return *this;
 }
 
@@ -139,7 +140,14 @@ ResourceTableBuilder& ResourceTableBuilder::SetOverlayable(const StringPiece& na
                                                            const OverlayableItem& overlayable) {
 
   ResourceName res_name = ParseNameOrDie(name);
-  CHECK(table_->SetOverlayable(res_name, overlayable, GetDiagnostics()));
+  CHECK(table_->AddResource(
+      NewResourceBuilder(res_name).SetOverlayable(overlayable).SetAllowMangled(true).Build(),
+      GetDiagnostics()));
+  return *this;
+}
+
+ResourceTableBuilder& ResourceTableBuilder::Add(NewResource&& res) {
+  CHECK(table_->AddResource(std::move(res), GetDiagnostics()));
   return *this;
 }
 

tools/aapt2/test/Builders.h

@@ -39,7 +39,6 @@ class ResourceTableBuilder {
  public:
   ResourceTableBuilder() = default;
 
-  ResourceTableBuilder& SetPackageId(const android::StringPiece& package_name, uint8_t id);
   ResourceTableBuilder& AddSimple(const android::StringPiece& name, const ResourceId& id = {});
   ResourceTableBuilder& AddSimple(const android::StringPiece& name,
                                   const android::ConfigDescription& config,
@@ -75,6 +74,7 @@ class ResourceTableBuilder {
                                        Visibility::Level level, bool allow_new = false);
   ResourceTableBuilder& SetOverlayable(const android::StringPiece& name,
                                        const OverlayableItem& overlayable);
+  ResourceTableBuilder& Add(NewResource&& res);
 
   StringPool* string_pool();
   std::unique_ptr<ResourceTable> Build();