From 06d3e8fec7e2b29f99d755bee849023d88957953 Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Thu, 5 Jan 2017 17:03:55 -0800 Subject: [PATCH] libandroidfw: Revert null check in ApplyStyle The out parameter `out_indices` is expected to be non-null and so the extra null check adds a cost to performance and opens the door to misusing the API by not supplying `out_indices`. Test: make libandroidfw_tests Change-Id: Ie66fd837c5e24ec2838156e7b67f54c15cd27933 --- libs/androidfw/AttributeResolution.cpp | 9 ++--- .../include/androidfw/AttributeResolution.h | 6 ++++ libs/androidfw/tests/AttributeResolution_test.cpp | 38 ++++++++++------------ 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/libs/androidfw/AttributeResolution.cpp b/libs/androidfw/AttributeResolution.cpp index f5aef05e8842..d433b90f14f0 100644 --- a/libs/androidfw/AttributeResolution.cpp +++ b/libs/androidfw/AttributeResolution.cpp @@ -418,8 +418,10 @@ void ApplyStyle(ResTable::Theme* theme, ResXMLParser* xml_parser, uint32_t def_s out_values[STYLE_CHANGING_CONFIGURATIONS] = type_set_flags; out_values[STYLE_DENSITY] = config.density; - if (out_indices != nullptr && value.dataType != Res_value::TYPE_NULL) { + if (value.dataType != Res_value::TYPE_NULL) { indices_idx++; + + // out_indices must NOT be nullptr. out_indices[indices_idx] = ii; } @@ -428,9 +430,8 @@ void ApplyStyle(ResTable::Theme* theme, ResXMLParser* xml_parser, uint32_t def_s res.unlock(); - if (out_indices != nullptr) { - out_indices[0] = indices_idx; - } + // out_indices must NOT be nullptr. + out_indices[0] = indices_idx; } bool RetrieveAttributes(const ResTable* res, ResXMLParser* xml_parser, diff --git a/libs/androidfw/include/androidfw/AttributeResolution.h b/libs/androidfw/include/androidfw/AttributeResolution.h index 8d5ff461444e..69b760414846 100644 --- a/libs/androidfw/include/androidfw/AttributeResolution.h +++ b/libs/androidfw/include/androidfw/AttributeResolution.h @@ -40,14 +40,20 @@ enum { // TODO(adamlesinski): Run performance tests against these methods and a new, single method // that uses all the sources and branches to the right ones within the inner loop. +// `out_values` must NOT be nullptr. +// `out_indices` may be nullptr. bool ResolveAttrs(ResTable::Theme* theme, uint32_t def_style_attr, uint32_t def_style_res, uint32_t* src_values, size_t src_values_length, uint32_t* attrs, size_t attrs_length, uint32_t* out_values, uint32_t* out_indices); +// `out_values` must NOT be nullptr. +// `out_indices` is NOT optional and must NOT be nullptr. void ApplyStyle(ResTable::Theme* theme, ResXMLParser* xml_parser, uint32_t def_style_attr, uint32_t def_style_res, const uint32_t* attrs, size_t attrs_length, uint32_t* out_values, uint32_t* out_indices); +// `out_values` must NOT be nullptr. +// `out_indices` may be nullptr. bool RetrieveAttributes(const ResTable* res, ResXMLParser* xml_parser, uint32_t* attrs, size_t attrs_length, uint32_t* out_values, uint32_t* out_indices); diff --git a/libs/androidfw/tests/AttributeResolution_test.cpp b/libs/androidfw/tests/AttributeResolution_test.cpp index d417aba1c595..75505171133c 100644 --- a/libs/androidfw/tests/AttributeResolution_test.cpp +++ b/libs/androidfw/tests/AttributeResolution_test.cpp @@ -16,6 +16,8 @@ #include "androidfw/AttributeResolution.h" +#include + #include "android-base/file.h" #include "android-base/logging.h" #include "android-base/macros.h" @@ -67,15 +69,13 @@ TEST_F(AttributeResolutionTest, Theme) { ResTable::Theme theme(table_); ASSERT_EQ(NO_ERROR, theme.applyStyle(R::style::StyleTwo)); - uint32_t attrs[] = {R::attr::attr_one, R::attr::attr_two, R::attr::attr_three, - R::attr::attr_four}; - std::vector values; - values.resize(arraysize(attrs) * 6); + std::array attrs{ + {R::attr::attr_one, R::attr::attr_two, R::attr::attr_three, R::attr::attr_four}}; + std::array values; ASSERT_TRUE(ResolveAttrs(&theme, 0 /*def_style_attr*/, 0 /*def_style_res*/, - nullptr /*src_values*/, 0 /*src_values_length*/, - attrs, arraysize(attrs), values.data(), - nullptr /*out_indices*/)); + nullptr /*src_values*/, 0 /*src_values_length*/, attrs.data(), + attrs.size(), values.data(), nullptr /*out_indices*/)); const uint32_t public_flag = ResTable_typeSpec::SPEC_PUBLIC; @@ -112,13 +112,12 @@ TEST_F(AttributeResolutionTest, Theme) { } TEST_F(AttributeResolutionXmlTest, XmlParser) { - uint32_t attrs[] = {R::attr::attr_one, R::attr::attr_two, R::attr::attr_three, - R::attr::attr_four}; - std::vector values; - values.resize(arraysize(attrs) * 6); + std::array attrs{ + {R::attr::attr_one, R::attr::attr_two, R::attr::attr_three, R::attr::attr_four}}; + std::array values; - ASSERT_TRUE(RetrieveAttributes(&table_, &xml_parser_, attrs, arraysize(attrs), - values.data(), nullptr /*out_indices*/)); + ASSERT_TRUE(RetrieveAttributes(&table_, &xml_parser_, attrs.data(), attrs.size(), values.data(), + nullptr /*out_indices*/)); uint32_t* values_cursor = values.data(); EXPECT_EQ(Res_value::TYPE_NULL, values_cursor[STYLE_TYPE]); @@ -157,14 +156,13 @@ TEST_F(AttributeResolutionXmlTest, ThemeAndXmlParser) { ResTable::Theme theme(table_); ASSERT_EQ(NO_ERROR, theme.applyStyle(R::style::StyleTwo)); - uint32_t attrs[] = {R::attr::attr_one, R::attr::attr_two, R::attr::attr_three, - R::attr::attr_four, R::attr::attr_five}; - std::vector values; - values.resize(arraysize(attrs) * 6); + std::array attrs{{R::attr::attr_one, R::attr::attr_two, R::attr::attr_three, + R::attr::attr_four, R::attr::attr_five}}; + std::array values; + std::array indices; - ApplyStyle(&theme, &xml_parser_, 0 /*def_style_attr*/, - 0 /*def_style_res*/, attrs, arraysize(attrs), - values.data(), nullptr /*out_indices*/); + ApplyStyle(&theme, &xml_parser_, 0 /*def_style_attr*/, 0 /*def_style_res*/, attrs.data(), + attrs.size(), values.data(), indices.data()); const uint32_t public_flag = ResTable_typeSpec::SPEC_PUBLIC; -- 2.11.0