Merge "AAPT2: Fix R.java styleable + indices ordering" into oc-mr1-dev

tools/aapt2/java/ClassDefinition.cpp

@@ -41,18 +41,21 @@ void MethodDefinition::WriteToStream(const StringPiece& prefix, bool final,
 
 ClassDefinition::Result ClassDefinition::AddMember(std::unique_ptr<ClassMember> member) {
   Result result = Result::kAdded;
-  auto iter = members_.find(member);
-  if (iter != members_.end()) {
-    members_.erase(iter);
+  auto iter = indexed_members_.find(member->GetName());
+  if (iter != indexed_members_.end()) {
+    // Overwrite the entry.
+    ordered_members_[iter->second].reset();
     result = Result::kOverridden;
   }
-  members_.insert(std::move(member));
+
+  indexed_members_[member->GetName()] = ordered_members_.size();
+  ordered_members_.push_back(std::move(member));
   return result;
 }
 
 bool ClassDefinition::empty() const {
-  for (const std::unique_ptr<ClassMember>& member : members_) {
-    if (!member->empty()) {
+  for (const std::unique_ptr<ClassMember>& member : ordered_members_) {
+    if (member != nullptr && !member->empty()) {
       return false;
     }
   }
@@ -61,7 +64,7 @@ bool ClassDefinition::empty() const {
 
 void ClassDefinition::WriteToStream(const StringPiece& prefix, bool final,
                                     std::ostream* out) const {
-  if (members_.empty() && !create_if_empty_) {
+  if (empty() && !create_if_empty_) {
     return;
   }
 
@@ -76,9 +79,14 @@ void ClassDefinition::WriteToStream(const StringPiece& prefix, bool final,
   std::string new_prefix = prefix.to_string();
   new_prefix.append(kIndent);
 
-  for (const std::unique_ptr<ClassMember>& member : members_) {
-    member->WriteToStream(new_prefix, final, out);
-    *out << "\n";
+  for (const std::unique_ptr<ClassMember>& member : ordered_members_) {
+    // There can be nullptr members when a member is added to the ClassDefinition
+    // and takes precedence over a previous member with the same name. The overridden member is
+    // set to nullptr.
+    if (member != nullptr) {
+      member->WriteToStream(new_prefix, final, out);
+      *out << "\n";
+    }
   }
 
   *out << prefix << "}";

tools/aapt2/java/ClassDefinition.h

@@ -18,8 +18,9 @@
 #define AAPT_JAVA_CLASSDEFINITION_H
 
 #include <ostream>
-#include <set>
 #include <string>
+#include <unordered_map>
+#include <vector>
 
 #include "android-base/macros.h"
 #include "androidfw/StringPiece.h"
@@ -73,21 +74,18 @@ class PrimitiveMember : public ClassMember {
   void WriteToStream(const android::StringPiece& prefix, bool final,
                      std::ostream* out) const override {
     ClassMember::WriteToStream(prefix, final, out);
-
-    *out << prefix << "public static " << (final ? "final " : "") << "int "
-         << name_ << "=" << val_ << ";";
+    *out << prefix << "public static " << (final ? "final " : "") << "int " << name_ << "=" << val_
+         << ";";
   }
 
  private:
+  DISALLOW_COPY_AND_ASSIGN(PrimitiveMember);
+
   std::string name_;
   T val_;
-
-  DISALLOW_COPY_AND_ASSIGN(PrimitiveMember);
 };
 
-/**
- * Specialization for strings so they get the right type and are quoted with "".
- */
+// Specialization for strings so they get the right type and are quoted with "".
 template <>
 class PrimitiveMember<std::string> : public ClassMember {
  public:
@@ -111,10 +109,10 @@ class PrimitiveMember<std::string> : public ClassMember {
   }
 
  private:
+  DISALLOW_COPY_AND_ASSIGN(PrimitiveMember);
+
   std::string name_;
   std::string val_;
-
-  DISALLOW_COPY_AND_ASSIGN(PrimitiveMember);
 };
 
 using IntMember = PrimitiveMember<uint32_t>;
@@ -160,10 +158,10 @@ class PrimitiveArrayMember : public ClassMember {
   }
 
  private:
+  DISALLOW_COPY_AND_ASSIGN(PrimitiveArrayMember);
+
   std::string name_;
   std::vector<T> elements_;
-
-  DISALLOW_COPY_AND_ASSIGN(PrimitiveArrayMember);
 };
 
 using ResourceArrayMember = PrimitiveArrayMember<ResourceId>;
@@ -185,12 +183,16 @@ class MethodDefinition : public ClassMember {
   }
 
   // Even if the method is empty, we always want to write the method signature.
-  bool empty() const override { return false; }
+  bool empty() const override {
+    return false;
+  }
 
   void WriteToStream(const android::StringPiece& prefix, bool final,
                      std::ostream* out) const override;
 
  private:
+  DISALLOW_COPY_AND_ASSIGN(MethodDefinition);
+
   std::string signature_;
   std::vector<std::string> statements_;
 };
@@ -222,19 +224,13 @@ class ClassDefinition : public ClassMember {
                      std::ostream* out) const override;
 
  private:
-  struct ClassMemberCompare {
-    using T = std::unique_ptr<ClassMember>;
-    bool operator()(const T& a, const T& b) const {
-      return a->GetName() < b->GetName();
-    }
-  };
+  DISALLOW_COPY_AND_ASSIGN(ClassDefinition);
 
   std::string name_;
   ClassQualifier qualifier_;
   bool create_if_empty_;
-  std::set<std::unique_ptr<ClassMember>, ClassMemberCompare> members_;
-
-  DISALLOW_COPY_AND_ASSIGN(ClassDefinition);
+  std::vector<std::unique_ptr<ClassMember>> ordered_members_;
+  std::unordered_map<android::StringPiece, size_t> indexed_members_;
 };
 
 }  // namespace aapt

tools/aapt2/java/JavaClassGenerator_test.cpp

@@ -23,6 +23,8 @@
 #include "util/Util.h"
 
 using android::StringPiece;
+using ::testing::Lt;
+using ::testing::Ne;
 
 namespace aapt {
 
@@ -329,6 +331,53 @@ TEST(JavaClassGeneratorTest, CommentsForStyleablesAndNestedAttributesArePresent)
   EXPECT_NE(std::string::npos, actual.find(styleable.GetComment().data()));
 }
 
+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",
+                    test::StyleableBuilder()
+                        .AddItem("android:attr/background", ResourceId(0x01010000))
+                        .Build())
+          .AddValue("android:styleable/ActionBar.LayoutParams",
+                    test::StyleableBuilder()
+                        .AddItem("android:attr/layout_gravity", ResourceId(0x01010001))
+                        .Build())
+          .Build();
+
+  std::unique_ptr<IAaptContext> context =
+      test::ContextBuilder()
+          .AddSymbolSource(util::make_unique<ResourceTableSymbolSource>(table.get()))
+          .SetNameManglerPolicy(NameManglerPolicy{"android"})
+          .Build();
+
+  JavaClassGeneratorOptions options;
+  JavaClassGenerator generator(context.get(), table.get(), {});
+  std::stringstream out;
+  ASSERT_TRUE(generator.Generate("android", &out));
+  std::string output = out.str();
+
+  std::string::size_type actionbar_pos = output.find("int[] ActionBar");
+  ASSERT_THAT(actionbar_pos, Ne(std::string::npos));
+
+  std::string::size_type actionbar_background_pos = output.find("int ActionBar_background");
+  ASSERT_THAT(actionbar_background_pos, Ne(std::string::npos));
+
+  std::string::size_type actionbar_layout_params_pos = output.find("int[] ActionBar_LayoutParams");
+  ASSERT_THAT(actionbar_layout_params_pos, Ne(std::string::npos));
+
+  std::string::size_type actionbar_layout_params_layout_gravity_pos =
+      output.find("int ActionBar_LayoutParams_layout_gravity");
+  ASSERT_THAT(actionbar_layout_params_layout_gravity_pos, Ne(std::string::npos));
+
+  EXPECT_THAT(actionbar_pos, Lt(actionbar_background_pos));
+  EXPECT_THAT(actionbar_pos, Lt(actionbar_layout_params_pos));
+  EXPECT_THAT(actionbar_background_pos, Lt(actionbar_layout_params_pos));
+  EXPECT_THAT(actionbar_layout_params_pos, Lt(actionbar_layout_params_layout_gravity_pos));
+}
+
 TEST(JavaClassGeneratorTest, CommentsForRemovedAttributesAreNotPresentInClass) {
   Attribute attr(false);
   attr.SetComment(StringPiece("removed"));