[aapt2] fix the comments from older review

- add a test case for the null last entry in sparse types
- add comments explaining what mLength is for sparse types
- remember if the type is sparse in a member
- pragma once

Test: atest libandroidfw_tests
Flag: EXEMPT bugfix / refactor
Change-Id: I7c2e516e8887f5fd1c2ab35b20fb49864b6761b6

3 files changed, 29 insertions, 13 deletions

diff --git a/libs/androidfw/TypeWrappers.cpp b/libs/androidfw/TypeWrappers.cpp
index 970463492c1a..2a20106b6ee1 100644
--- a/libs/androidfw/TypeWrappers.cpp
+++ b/libs/androidfw/TypeWrappers.cpp
@@ -18,8 +18,11 @@
 
 namespace android {
 
-TypeVariant::TypeVariant(const ResTable_type* data) : data(data), mLength(dtohl(data->entryCount)) {
-    if (data->flags & ResTable_type::FLAG_SPARSE) {
+TypeVariant::TypeVariant(const ResTable_type* data)
+    : data(data)
+    , mLength(dtohl(data->entryCount))
+    , mSparse(data->flags & ResTable_type::FLAG_SPARSE) {
+    if (mSparse) {
         const uint32_t entryCount = dtohl(data->entryCount);
         const uintptr_t containerEnd = reinterpret_cast<uintptr_t>(data) + dtohl(data->header.size);
         const uint32_t* const entryIndices = reinterpret_cast<const uint32_t*>(
@@ -40,18 +43,18 @@ TypeVariant::iterator& TypeVariant::iterator::operator++() {
         mIndex = mTypeVariant->mLength;
     }
 
-    const ResTable_type* type = mTypeVariant->data;
-    if ((type->flags & ResTable_type::FLAG_SPARSE) == 0) {
+    if (!mTypeVariant->mSparse) {
       return *this;
     }
 
     // Need to adjust |mSparseIndex| as well if we've passed its current element.
+    const ResTable_type* type = mTypeVariant->data;
     const uint32_t entryCount = dtohl(type->entryCount);
-    const auto entryIndices = reinterpret_cast<const uint32_t*>(
-        reinterpret_cast<uintptr_t>(type) + dtohs(type->header.headerSize));
     if (mSparseIndex >= entryCount) {
       return *this; // done
     }
+    const auto entryIndices = reinterpret_cast<const uint32_t*>(
+        reinterpret_cast<uintptr_t>(type) + dtohs(type->header.headerSize));
     const auto element = (const ResTable_sparseTypeEntry*)(entryIndices + mSparseIndex);
     if (mIndex > dtohs(element->idx)) {
       ++mSparseIndex;
@@ -79,7 +82,7 @@ const ResTable_entry* TypeVariant::iterator::operator*() const {
     }
 
     uint32_t entryOffset;
-    if (type->flags & ResTable_type::FLAG_SPARSE) {
+    if (mTypeVariant->mSparse) {
       if (mSparseIndex >= entryCount) {
         return nullptr;
       }
diff --git a/libs/androidfw/include/androidfw/TypeWrappers.h b/libs/androidfw/include/androidfw/TypeWrappers.h
index db641b78a4e4..d901af3c908b 100644
--- a/libs/androidfw/include/androidfw/TypeWrappers.h
+++ b/libs/androidfw/include/androidfw/TypeWrappers.h
@@ -14,8 +14,7 @@
  * limitations under the License.
  */
 
-#ifndef __TYPE_WRAPPERS_H
-#define __TYPE_WRAPPERS_H
+#pragma once
 
 #include <androidfw/ResourceTypes.h>
 #include <utils/ByteOrder.h>
@@ -54,7 +53,7 @@ struct TypeVariant {
         enum class Kind { Begin, End };
         iterator(const TypeVariant* tv, Kind kind)
             : mTypeVariant(tv) {
-          mSparseIndex = mIndex = kind == Kind::Begin ? 0 : tv->mLength;
+          mSparseIndex = mIndex = (kind == Kind::Begin ? 0 : tv->mLength);
           // mSparseIndex here is technically past the number of sparse entries, but it is still
           // ok as it is enough to infer that this is the end iterator.
         }
@@ -75,9 +74,11 @@ struct TypeVariant {
     const ResTable_type* data;
 
 private:
-    size_t mLength;
+    // For a dense table, this is the number of the elements.
+    // For a sparse table, this is the index of the last element + 1.
+    // In both cases, it can be used for iteration as the upper loop bound as in |i < mLength|.
+    uint32_t mLength;
+    bool mSparse;
 };
 
 } // namespace android
-
-#endif // __TYPE_WRAPPERS_H
diff --git a/libs/androidfw/tests/TypeWrappers_test.cpp b/libs/androidfw/tests/TypeWrappers_test.cpp
index d66e05805484..69c24c5d8956 100644
--- a/libs/androidfw/tests/TypeWrappers_test.cpp
+++ b/libs/androidfw/tests/TypeWrappers_test.cpp
@@ -121,6 +121,7 @@ TEST(TypeVariantIteratorTest, shouldIterateOverTypeWithoutErrors) {
     values.push_back(std::nullopt);
     values.push_back(std::nullopt);
     values.push_back(Res_value{ sizeof(Res_value), 0, Res_value::TYPE_STRING, 0x87654321});
+    values.push_back(std::nullopt);
 
     // test for combinations of compact_entry and short_offsets
     for (size_t i = 0; i < 8; i++) {
@@ -191,6 +192,17 @@ TEST(TypeVariantIteratorTest, shouldIterateOverTypeWithoutErrors) {
 
         ++iter;
 
+        ASSERT_EQ(uint32_t(9), iter.index());
+        ASSERT_TRUE(NULL == *iter);
+        if (sparse) {
+          // Sparse iterator doesn't know anything beyond the last entry.
+          ASSERT_EQ(v.endEntries(), iter);
+        } else {
+          ASSERT_NE(v.endEntries(), iter);
+        }
+
+        ++iter;
+
         ASSERT_EQ(v.endEntries(), iter);
     }
 }