idmap2: replace std::pair<bool, T> with Result<T>

Introduce a new type Result<T> to indicate if an operation succeeded or
not, and if it did, to hold the return value of the operation. This is
the same as how std::pair<bool, T> is already used in the codebase, so
replace all instances with Result<T> to improve clarity.

Result<T> is simply an alias for std::optional<T>. The difference is
semantic: use Result<T> as the return value for functions that can fail,
use std::optional<T> when values are truly optional. This is modelled
after Rust's std::result and std::option.

A future change may graduate Result<T> to a proper class which can hold
additional details on why an operation failed, such as a string or an
error code. As a special case, continue to use std::unique_ptr<T>
instead of Result<std::unique_ptr<T>> for now: the latter would increase
code complexity without added benefit.

Test: make idmap2_tests
Change-Id: I2a8355107ed2b6485409e5e655a84cf1e20b9911

diff --git a/cmds/idmap2/idmap2/Lookup.cpp b/cmds/idmap2/idmap2/Lookup.cpp
index 020c443..8d0cee5 100644 (file)
--- a/cmds/idmap2/idmap2/Lookup.cpp
+++ b/cmds/idmap2/idmap2/Lookup.cpp
@@ -36,6 +36,7 @@
 
 #include "idmap2/CommandLineOptions.h"
 #include "idmap2/Idmap.h"
+#include "idmap2/Result.h"
 #include "idmap2/Xml.h"
 #include "idmap2/ZipFile.h"
 
@@ -53,39 +54,40 @@ using android::base::StringPrintf;
 using android::idmap2::CommandLineOptions;
 using android::idmap2::IdmapHeader;
 using android::idmap2::ResourceId;
+using android::idmap2::Result;
 using android::idmap2::Xml;
 using android::idmap2::ZipFile;
 using android::util::Utf16ToUtf8;
 
 namespace {
-std::pair<bool, ResourceId> WARN_UNUSED ParseResReference(const AssetManager2& am,
-                                                          const std::string& res,
-                                                          const std::string& fallback_package) {
+
+Result<ResourceId> WARN_UNUSED ParseResReference(const AssetManager2& am, const std::string& res,
+                                                 const std::string& fallback_package) {
   // first, try to parse as a hex number
   char* endptr = nullptr;
   ResourceId resid;
   resid = strtol(res.c_str(), &endptr, 16);
   if (*endptr == '\0') {
-    return std::make_pair(true, resid);
+    return {resid};
   }
 
   // next, try to parse as a package:type/name string
   resid = am.GetResourceId(res, "", fallback_package);
   if (is_valid_resid(resid)) {
-    return std::make_pair(true, resid);
+    return {resid};
   }
 
   // end of the road: res could not be parsed
-  return std::make_pair(false, 0);
+  return {};
 }
 
-std::pair<bool, std::string> WARN_UNUSED GetValue(const AssetManager2& am, ResourceId resid) {
+Result<std::string> WARN_UNUSED GetValue(const AssetManager2& am, ResourceId resid) {
   Res_value value;
   ResTable_config config;
   uint32_t flags;
   ApkAssetsCookie cookie = am.GetResource(resid, false, 0, &value, &config, &flags);
   if (cookie == kInvalidCookie) {
-    return std::make_pair(false, "");
+    return {};
   }
 
   std::string out;
@@ -123,31 +125,31 @@ std::pair<bool, std::string> WARN_UNUSED GetValue(const AssetManager2& am, Resou
       out.append(StringPrintf("dataType=0x%02x data=0x%08x", value.dataType, value.data));
       break;
   }
-  return std::make_pair(true, out);
+  return {out};
 }
 
-std::pair<bool, std::string> GetTargetPackageNameFromManifest(const std::string& apk_path) {
+Result<std::string> GetTargetPackageNameFromManifest(const std::string& apk_path) {
   const auto zip = ZipFile::Open(apk_path);
   if (!zip) {
-    return std::make_pair(false, "");
+    return {};
   }
   const auto entry = zip->Uncompress("AndroidManifest.xml");
   if (!entry) {
-    return std::make_pair(false, "");
+    return {};
   }
   const auto xml = Xml::Create(entry->buf, entry->size);
   if (!xml) {
-    return std::make_pair(false, "");
+    return {};
   }
   const auto tag = xml->FindTag("overlay");
   if (!tag) {
-    return std::make_pair(false, "");
+    return {};
   }
   const auto iter = tag->find("targetPackage");
   if (iter == tag->end()) {
-    return std::make_pair(false, "");
+    return {};
   }
-  return std::make_pair(true, iter->second);
+  return {iter->second};
 }
 }  // namespace
 
@@ -195,14 +197,14 @@ bool Lookup(const std::vector<std::string>& args, std::ostream& out_error) {
       }
       apk_assets.push_back(std::move(target_apk));
 
-      bool lookup_ok;
-      std::tie(lookup_ok, target_package_name) =
+      const Result<std::string> package_name =
           GetTargetPackageNameFromManifest(idmap_header->GetOverlayPath().to_string());
-      if (!lookup_ok) {
+      if (!package_name) {
         out_error << "error: failed to parse android:targetPackage from overlay manifest"
                   << std::endl;
         return false;
       }
+      target_package_name = *package_name;
     } else if (target_path != idmap_header->GetTargetPath()) {
       out_error << "error: different target APKs (expected target APK " << target_path << " but "
                 << idmap_path << " has target APK " << idmap_header->GetTargetPath() << ")"
@@ -227,21 +229,18 @@ bool Lookup(const std::vector<std::string>& args, std::ostream& out_error) {
   am.SetApkAssets(raw_pointer_apk_assets);
   am.SetConfiguration(config);
 
-  ResourceId resid;
-  bool lookup_ok;
-  std::tie(lookup_ok, resid) = ParseResReference(am, resid_str, target_package_name);
-  if (!lookup_ok) {
+  const Result<ResourceId> resid = ParseResReference(am, resid_str, target_package_name);
+  if (!resid) {
     out_error << "error: failed to parse resource ID" << std::endl;
     return false;
   }
 
-  std::string value;
-  std::tie(lookup_ok, value) = GetValue(am, resid);
-  if (!lookup_ok) {
-    out_error << StringPrintf("error: resource 0x%08x not found", resid) << std::endl;
+  const Result<std::string> value = GetValue(am, *resid);
+  if (!value) {
+    out_error << StringPrintf("error: resource 0x%08x not found", *resid) << std::endl;
     return false;
   }
-  std::cout << value << std::endl;
+  std::cout << *value << std::endl;
 
   return true;
 }

diff --git a/cmds/idmap2/include/idmap2/ResourceUtils.h b/cmds/idmap2/include/idmap2/ResourceUtils.h
index 88a835b..d106f19 100644 (file)
--- a/cmds/idmap2/include/idmap2/ResourceUtils.h
+++ b/cmds/idmap2/include/idmap2/ResourceUtils.h
@@ -18,19 +18,18 @@
 #define IDMAP2_INCLUDE_IDMAP2_RESOURCEUTILS_H_
 
 #include <string>
-#include <utility>
 
 #include "android-base/macros.h"
 #include "androidfw/AssetManager2.h"
 
 #include "idmap2/Idmap.h"
+#include "idmap2/Result.h"
 
 namespace android {
 namespace idmap2 {
 namespace utils {
 
-std::pair<bool, std::string> WARN_UNUSED ResToTypeEntryName(const AssetManager2& am,
-                                                            ResourceId resid);
+Result<std::string> WARN_UNUSED ResToTypeEntryName(const AssetManager2& am, ResourceId resid);
 
 }  // namespace utils
 }  // namespace idmap2

diff --git a/cmds/idmap2/include/idmap2/Result.h b/cmds/idmap2/include/idmap2/Result.h
new file mode 100644 (file)
index 0000000..6189ea3
--- /dev/null
+++ b/cmds/idmap2/include/idmap2/Result.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright (C) 2018 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef IDMAP2_INCLUDE_IDMAP2_RESULT_H_
+#define IDMAP2_INCLUDE_IDMAP2_RESULT_H_
+
+#include <optional>
+
+namespace android::idmap2 {
+
+template <typename T>
+using Result = std::optional<T>;
+
+static constexpr std::nullopt_t kResultError = std::nullopt;
+
+}  // namespace android::idmap2
+
+#endif  // IDMAP2_INCLUDE_IDMAP2_RESULT_H_

diff --git a/cmds/idmap2/include/idmap2/ZipFile.h b/cmds/idmap2/include/idmap2/ZipFile.h
index 328bd36..9edbbe0 100644 (file)
--- a/cmds/idmap2/include/idmap2/ZipFile.h
+++ b/cmds/idmap2/include/idmap2/ZipFile.h
@@ -19,10 +19,10 @@
 
 #include <memory>
 #include <string>
-#include <utility>
 
 #include "android-base/macros.h"
 #include "ziparchive/zip_archive.h"
+#include "idmap2/Result.h"
 
 namespace android {
 namespace idmap2 {
@@ -43,7 +43,7 @@ class ZipFile {
   static std::unique_ptr<const ZipFile> Open(const std::string& path);
 
   std::unique_ptr<const MemoryChunk> Uncompress(const std::string& entryPath) const;
-  std::pair<bool, uint32_t> Crc(const std::string& entryPath) const;
+  Result<uint32_t> Crc(const std::string& entryPath) const;
 
   ~ZipFile();
 

diff --git a/cmds/idmap2/libidmap2/Idmap.cpp b/cmds/idmap2/libidmap2/Idmap.cpp
index 5a47e30..1ef3267 100644 (file)
--- a/cmds/idmap2/libidmap2/Idmap.cpp
+++ b/cmds/idmap2/libidmap2/Idmap.cpp
@@ -33,6 +33,7 @@
 
 #include "idmap2/Idmap.h"
 #include "idmap2/ResourceUtils.h"
+#include "idmap2/Result.h"
 #include "idmap2/ZipFile.h"
 
 namespace android {
@@ -143,18 +144,16 @@ bool IdmapHeader::IsUpToDate(std::ostream& out_error) const {
     return false;
   }
 
-  bool status;
-  uint32_t target_crc;
-  std::tie(status, target_crc) = target_zip->Crc("resources.arsc");
-  if (!status) {
+  Result<uint32_t> target_crc = target_zip->Crc("resources.arsc");
+  if (!target_crc) {
     out_error << "error: failed to get target crc" << std::endl;
     return false;
   }
 
-  if (target_crc_ != target_crc) {
+  if (target_crc_ != *target_crc) {
     out_error << base::StringPrintf(
                      "error: bad target crc: idmap version 0x%08x, file system version 0x%08x",
-                     target_crc_, target_crc)
+                     target_crc_, *target_crc)
               << std::endl;
     return false;
   }
@@ -165,17 +164,16 @@ bool IdmapHeader::IsUpToDate(std::ostream& out_error) const {
     return false;
   }
 
-  uint32_t overlay_crc;
-  std::tie(status, overlay_crc) = overlay_zip->Crc("resources.arsc");
-  if (!status) {
+  Result<uint32_t> overlay_crc = overlay_zip->Crc("resources.arsc");
+  if (!overlay_crc) {
     out_error << "error: failed to get overlay crc" << std::endl;
     return false;
   }
 
-  if (overlay_crc_ != overlay_crc) {
+  if (overlay_crc_ != *overlay_crc) {
     out_error << base::StringPrintf(
                      "error: bad overlay crc: idmap version 0x%08x, file system version 0x%08x",
-                     overlay_crc_, overlay_crc)
+                     overlay_crc_, *overlay_crc)
               << std::endl;
     return false;
   }
@@ -322,17 +320,20 @@ std::unique_ptr<const Idmap> Idmap::FromApkAssets(const std::string& target_apk_
   std::unique_ptr<IdmapHeader> header(new IdmapHeader());
   header->magic_ = kIdmapMagic;
   header->version_ = kIdmapCurrentVersion;
-  bool crc_status;
-  std::tie(crc_status, header->target_crc_) = target_zip->Crc("resources.arsc");
-  if (!crc_status) {
+
+  Result<uint32_t> crc = target_zip->Crc("resources.arsc");
+  if (!crc) {
     out_error << "error: failed to get zip crc for target" << std::endl;
     return nullptr;
   }
-  std::tie(crc_status, header->overlay_crc_) = overlay_zip->Crc("resources.arsc");
-  if (!crc_status) {
+  header->target_crc_ = *crc;
+
+  crc = overlay_zip->Crc("resources.arsc");
+  if (!crc) {
     out_error << "error: failed to get zip crc for overlay" << std::endl;
     return nullptr;
   }
+  header->overlay_crc_ = *crc;
 
   if (target_apk_path.size() > sizeof(header->target_path_)) {
     out_error << "error: target apk path \"" << target_apk_path << "\" longer that maximum size "
@@ -358,15 +359,14 @@ std::unique_ptr<const Idmap> Idmap::FromApkAssets(const std::string& target_apk_
   const auto end = overlay_pkg->end();
   for (auto iter = overlay_pkg->begin(); iter != end; ++iter) {
     const ResourceId overlay_resid = *iter;
-    bool lookup_ok;
-    std::string name;
-    std::tie(lookup_ok, name) = utils::ResToTypeEntryName(overlay_asset_manager, overlay_resid);
-    if (!lookup_ok) {
+    Result<std::string> name = utils::ResToTypeEntryName(overlay_asset_manager, overlay_resid);
+    if (!name) {
       continue;
     }
     // prepend "<package>:" to turn name into "<package>:<type>/<name>"
-    name = base::StringPrintf("%s:%s", target_pkg->GetPackageName().c_str(), name.c_str());
-    const ResourceId target_resid = NameToResid(target_asset_manager, name);
+    const std::string full_name =
+        base::StringPrintf("%s:%s", target_pkg->GetPackageName().c_str(), name->c_str());
+    const ResourceId target_resid = NameToResid(target_asset_manager, full_name);
     if (target_resid == 0) {
       continue;
     }

diff --git a/cmds/idmap2/libidmap2/PrettyPrintVisitor.cpp b/cmds/idmap2/libidmap2/PrettyPrintVisitor.cpp
index 492e6f0..fb3bc5b 100644 (file)
--- a/cmds/idmap2/libidmap2/PrettyPrintVisitor.cpp
+++ b/cmds/idmap2/libidmap2/PrettyPrintVisitor.cpp
@@ -15,7 +15,6 @@
  */
 
 #include <string>
-#include <utility>
 
 #include "android-base/macros.h"
 #include "android-base/stringprintf.h"
@@ -23,6 +22,7 @@
 
 #include "idmap2/PrettyPrintVisitor.h"
 #include "idmap2/ResourceUtils.h"
+#include "idmap2/Result.h"
 
 namespace android {
 namespace idmap2 {
@@ -63,11 +63,9 @@ void PrettyPrintVisitor::visit(const IdmapData::TypeEntry& te) {
 
     stream_ << base::StringPrintf("0x%08x -> 0x%08x", target_resid, overlay_resid);
     if (target_package_loaded) {
-      bool lookup_ok;
-      std::string name;
-      std::tie(lookup_ok, name) = utils::ResToTypeEntryName(target_am_, target_resid);
-      if (lookup_ok) {
-        stream_ << " " << name;
+      Result<std::string> name = utils::ResToTypeEntryName(target_am_, target_resid);
+      if (name) {
+        stream_ << " " << *name;
       }
     }
     stream_ << std::endl;

diff --git a/cmds/idmap2/libidmap2/RawPrintVisitor.cpp b/cmds/idmap2/libidmap2/RawPrintVisitor.cpp
index 57cfc8e..7c24445 100644 (file)
--- a/cmds/idmap2/libidmap2/RawPrintVisitor.cpp
+++ b/cmds/idmap2/libidmap2/RawPrintVisitor.cpp
@@ -16,7 +16,6 @@
 
 #include <cstdarg>
 #include <string>
-#include <utility>
 
 #include "android-base/macros.h"
 #include "android-base/stringprintf.h"
@@ -24,6 +23,7 @@
 
 #include "idmap2/RawPrintVisitor.h"
 #include "idmap2/ResourceUtils.h"
+#include "idmap2/Result.h"
 
 using android::ApkAssets;
 
@@ -75,14 +75,13 @@ void RawPrintVisitor::visit(const IdmapData::TypeEntry& te) {
       const ResourceId target_resid =
           RESID(last_seen_package_id_, te.GetTargetTypeId(), te.GetEntryOffset() + i);
       const ResourceId overlay_resid = RESID(last_seen_package_id_, te.GetOverlayTypeId(), entry);
-      bool lookup_ok = false;
-      std::string name;
+      Result<std::string> name;
       if (target_package_loaded) {
-        std::tie(lookup_ok, name) = utils::ResToTypeEntryName(target_am_, target_resid);
+        name = utils::ResToTypeEntryName(target_am_, target_resid);
       }
-      if (lookup_ok) {
+      if (name) {
         print(static_cast<uint32_t>(entry), "0x%08x -> 0x%08x %s", target_resid, overlay_resid,
-              name.c_str());
+              name->c_str());
       } else {
         print(static_cast<uint32_t>(entry), "0x%08x -> 0x%08x", target_resid, overlay_resid);
       }

diff --git a/cmds/idmap2/libidmap2/ResourceUtils.cpp b/cmds/idmap2/libidmap2/ResourceUtils.cpp
index e98f843..5c89783 100644 (file)
--- a/cmds/idmap2/libidmap2/ResourceUtils.cpp
+++ b/cmds/idmap2/libidmap2/ResourceUtils.cpp
@@ -15,12 +15,12 @@
  */
 
 #include <string>
-#include <utility>
 
 #include "androidfw/StringPiece.h"
 #include "androidfw/Util.h"
 
 #include "idmap2/ResourceUtils.h"
+#include "idmap2/Result.h"
 
 using android::StringPiece16;
 using android::util::Utf16ToUtf8;
@@ -29,11 +29,10 @@ namespace android {
 namespace idmap2 {
 namespace utils {
 
-std::pair<bool, std::string> WARN_UNUSED ResToTypeEntryName(const AssetManager2& am,
-                                                            ResourceId resid) {
+Result<std::string> WARN_UNUSED ResToTypeEntryName(const AssetManager2& am, ResourceId resid) {
   AssetManager2::ResourceName name;
   if (!am.GetResourceName(resid, &name)) {
-    return std::make_pair(false, "");
+    return {};
   }
   std::string out;
   if (name.type != nullptr) {
@@ -47,7 +46,7 @@ std::pair<bool, std::string> WARN_UNUSED ResToTypeEntryName(const AssetManager2&
   } else {
     out += Utf16ToUtf8(StringPiece16(name.entry16, name.entry_len));
   }
-  return std::make_pair(true, out);
+  return {out};
 }
 
 }  // namespace utils

diff --git a/cmds/idmap2/libidmap2/ZipFile.cpp b/cmds/idmap2/libidmap2/ZipFile.cpp
index 3f2079a..9fb611d 100644 (file)
--- a/cmds/idmap2/libidmap2/ZipFile.cpp
+++ b/cmds/idmap2/libidmap2/ZipFile.cpp
@@ -16,8 +16,8 @@
 
 #include <memory>
 #include <string>
-#include <utility>
 
+#include "idmap2/Result.h"
 #include "idmap2/ZipFile.h"
 
 namespace android {
@@ -57,10 +57,10 @@ std::unique_ptr<const MemoryChunk> ZipFile::Uncompress(const std::string& entryP
   return chunk;
 }
 
-std::pair<bool, uint32_t> ZipFile::Crc(const std::string& entryPath) const {
+Result<uint32_t> ZipFile::Crc(const std::string& entryPath) const {
   ::ZipEntry entry;
   int32_t status = ::FindEntry(handle_, ::ZipString(entryPath.c_str()), &entry);
-  return std::make_pair(status == 0, entry.crc32);
+  return status == 0 ? Result<uint32_t>(entry.crc32) : kResultError;
 }
 
 }  // namespace idmap2

diff --git a/cmds/idmap2/tests/ResourceUtilsTests.cpp b/cmds/idmap2/tests/ResourceUtilsTests.cpp
index 0547fa0..7f60d75 100644 (file)
--- a/cmds/idmap2/tests/ResourceUtilsTests.cpp
+++ b/cmds/idmap2/tests/ResourceUtilsTests.cpp
@@ -16,13 +16,13 @@
 
 #include <memory>
 #include <string>
-#include <utility>
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include "androidfw/ApkAssets.h"
 #include "idmap2/ResourceUtils.h"
+#include "idmap2/Result.h"
 
 #include "TestHelpers.h"
 
@@ -52,17 +52,14 @@ class ResourceUtilsTests : public Idmap2Tests {
 };
 
 TEST_F(ResourceUtilsTests, ResToTypeEntryName) {
-  bool lookup_ok;
-  std::string name;
-  std::tie(lookup_ok, name) = utils::ResToTypeEntryName(GetAssetManager(), 0x7f010000u);
-  ASSERT_TRUE(lookup_ok);
-  ASSERT_EQ(name, "integer/int1");
+  Result<std::string> name = utils::ResToTypeEntryName(GetAssetManager(), 0x7f010000u);
+  ASSERT_TRUE(name);
+  ASSERT_EQ(*name, "integer/int1");
 }
 
 TEST_F(ResourceUtilsTests, ResToTypeEntryNameNoSuchResourceId) {
-  bool lookup_ok;
-  std::tie(lookup_ok, std::ignore) = utils::ResToTypeEntryName(GetAssetManager(), 0x7f123456u);
-  ASSERT_FALSE(lookup_ok);
+  Result<std::string> name = utils::ResToTypeEntryName(GetAssetManager(), 0x7f123456u);
+  ASSERT_FALSE(name);
 }
 
 }  // namespace idmap2

diff --git a/cmds/idmap2/tests/ZipFileTests.cpp b/cmds/idmap2/tests/ZipFileTests.cpp
index a504d31..6e4a501 100644 (file)
--- a/cmds/idmap2/tests/ZipFileTests.cpp
+++ b/cmds/idmap2/tests/ZipFileTests.cpp
@@ -16,8 +16,8 @@
 
 #include <cstdio>  // fclose
 #include <string>
-#include <utility>
 
+#include "idmap2/Result.h"
 #include "idmap2/ZipFile.h"
 
 #include "gmock/gmock.h"
@@ -44,14 +44,12 @@ TEST(ZipFileTests, Crc) {
   auto zip = ZipFile::Open(GetTestDataPath() + "/target/target.apk");
   ASSERT_THAT(zip, NotNull());
 
-  bool status;
-  uint32_t crc;
-  std::tie(status, crc) = zip->Crc("AndroidManifest.xml");
-  ASSERT_TRUE(status);
-  ASSERT_EQ(crc, 0x762f3d24);
+  Result<uint32_t> crc = zip->Crc("AndroidManifest.xml");
+  ASSERT_TRUE(crc);
+  ASSERT_EQ(*crc, 0x762f3d24);
 
-  std::tie(status, std::ignore) = zip->Crc("does-not-exist");
-  ASSERT_FALSE(status);
+  Result<uint32_t> crc2 = zip->Crc("does-not-exist");
+  ASSERT_FALSE(crc2);
 }
 
 TEST(ZipFileTests, Uncompress) {