Merge pull request #11951 from Minty-Meeo/dolphin-tool-code-review-1

DolphinTool: Sensible changes

Source/Core/DolphinTool/ConvertCommand.cpp

@@ -340,8 +340,7 @@ ConvertCommand::ParseCompressionTypeString(const std::string& compression_str)
     return DiscIO::WIARVZCompressionType::LZMA2;
   else if (compression_str == "zstd")
     return DiscIO::WIARVZCompressionType::Zstd;
-  else
+  return std::nullopt;
-    return std::nullopt;
 }
 
 std::optional<DiscIO::BlobType> ConvertCommand::ParseFormatString(const std::string& format_str)
@@ -354,8 +353,6 @@ std::optional<DiscIO::BlobType> ConvertCommand::ParseFormatString(const std::str
     return DiscIO::BlobType::WIA;
   else if (format_str == "rvz")
     return DiscIO::BlobType::RVZ;
-  else
+  return std::nullopt;
-    return std::nullopt;
 }
-
 }  // namespace DolphinTool

Source/Core/DolphinTool/HeaderCommand.cpp

@@ -13,29 +13,29 @@ namespace DolphinTool
 {
 int HeaderCommand::Main(const std::vector<std::string>& args)
 {
-  auto parser = std::make_unique<optparse::OptionParser>();
+  optparse::OptionParser parser;
 
-  parser->usage("usage: header [options]...");
+  parser.usage("usage: header [options]...");
 
-  parser->add_option("-i", "--input")
+  parser.add_option("-i", "--input")
       .type("string")
       .action("store")
       .help("Path to disc image FILE.")
       .metavar("FILE");
 
-  parser->add_option("-b", "--block_size")
+  parser.add_option("-b", "--block_size")
       .action("store_true")
       .help("Optional. Print the block size of GCZ/WIA/RVZ formats, then exit.");
 
-  parser->add_option("-c", "--compression")
+  parser.add_option("-c", "--compression")
       .action("store_true")
       .help("Optional. Print the compression method of GCZ/WIA/RVZ formats, then exit.");
 
-  parser->add_option("-l", "--compression_level")
+  parser.add_option("-l", "--compression_level")
       .action("store_true")
       .help("Optional. Print the level of compression for WIA/RVZ formats, then exit.");
 
-  const optparse::Values& options = parser->parse_args(args);
+  const optparse::Values& options = parser.parse_args(args);
 
   // Validate options
   const std::string input_file_path = static_cast<const char*>(options.get("input"));
@@ -45,18 +45,17 @@ int HeaderCommand::Main(const std::vector<std::string>& args)
     return 1;
   }
 
-  bool enable_block_size = options.is_set_by_user("block_size");
+  const bool enable_block_size = options.is_set_by_user("block_size");
-  bool enable_compression_method = options.is_set_by_user("compression");
+  const bool enable_compression_method = options.is_set_by_user("compression");
-  bool enable_compression_level = options.is_set_by_user("compression_level");
+  const bool enable_compression_level = options.is_set_by_user("compression_level");
 
   // Open the blob reader, plus get blob type
-  std::shared_ptr<DiscIO::BlobReader> blob_reader = DiscIO::CreateBlobReader(input_file_path);
+  const std::unique_ptr<DiscIO::BlobReader> blob_reader = DiscIO::CreateBlobReader(input_file_path);
   if (!blob_reader)
   {
     std::cerr << "Error: Unable to open disc image" << std::endl;
     return 1;
   }
-  const DiscIO::BlobType blob_type = blob_reader->GetBlobType();
 
   if (enable_block_size || enable_compression_method || enable_compression_level)
   {
@@ -87,6 +86,7 @@ int HeaderCommand::Main(const std::vector<std::string>& args)
   }
   else
   {
+    const auto blob_type = blob_reader->GetBlobType();
     if (blob_type == DiscIO::BlobType::GCZ)
     {
       std::cout << "Block Size: " << blob_reader->GetBlockSize() << std::endl;
@@ -102,5 +102,4 @@ int HeaderCommand::Main(const std::vector<std::string>& args)
 
   return 0;
 }
-
 }  // namespace DolphinTool

Source/Core/DolphinTool/ToolMain.cpp

@@ -5,6 +5,7 @@
 #include <cstring>
 #include <iostream>
 #include <string>
+#include <string_view>
 #include <vector>
 
 #include "Common/Version.h"
@@ -33,12 +34,9 @@ int main(int argc, char* argv[])
   if (argc < 2)
     return PrintUsage(1);
 
-  std::vector<std::string> args(argv, argv + argc);
+  const std::string_view command_str = argv[1];
-
+  // Take off the program name and command selector before passing arguments down
-  std::string command_str = args.at(1);
+  const std::vector<std::string> args(argv + 2, argv + argc);
-
-  // Take off the command selector before passing arguments down
-  args.erase(args.begin(), args.begin() + 1);
 
   std::unique_ptr<DolphinTool::Command> command;
 

Source/Core/DolphinTool/VerifyCommand.cpp

@@ -10,29 +10,29 @@ namespace DolphinTool
 {
 int VerifyCommand::Main(const std::vector<std::string>& args)
 {
-  auto parser = std::make_unique<optparse::OptionParser>();
+  optparse::OptionParser parser;
 
-  parser->usage("usage: verify [options]...");
+  parser.usage("usage: verify [options]...");
 
-  parser->add_option("-u", "--user")
+  parser.add_option("-u", "--user")
       .action("store")
       .help("User folder path, required for temporary processing files. "
             "Will be automatically created if this option is not set.");
 
-  parser->add_option("-i", "--input")
+  parser.add_option("-i", "--input")
       .type("string")
       .action("store")
       .help("Path to disc image FILE.")
       .metavar("FILE");
 
-  parser->add_option("-a", "--algorithm")
+  parser.add_option("-a", "--algorithm")
       .type("string")
       .action("store")
       .help("Optional. Compute and print the digest using the selected algorithm, then exit. "
             "[%choices]")
       .choices({"crc32", "md5", "sha1"});
 
-  const optparse::Values& options = parser->parse_args(args);
+  const optparse::Values& options = parser.parse_args(args);
 
   // Initialize the dolphin user directory, required for temporary processing files
   // If this is not set, destructive file operations could occur due to path confusion
@@ -51,19 +51,15 @@ int VerifyCommand::Main(const std::vector<std::string>& args)
     return 1;
   }
 
-  std::optional<std::string> algorithm;
-  if (options.is_set("algorithm"))
-  {
-    algorithm = static_cast<const char*>(options.get("algorithm"));
-  }
-
   DiscIO::Hashes<bool> hashes_to_calculate{};
-  if (algorithm == std::nullopt)
+  const bool algorithm_is_set = options.is_set("algorithm");
+  if (!algorithm_is_set)
   {
     hashes_to_calculate = DiscIO::VolumeVerifier::GetDefaultHashesToCalculate();
   }
   else
   {
+    const std::string algorithm = static_cast<const char*>(options.get("algorithm"));
     if (algorithm == "crc32")
       hashes_to_calculate.crc32 = true;
     else if (algorithm == "md5")
@@ -80,7 +76,7 @@ int VerifyCommand::Main(const std::vector<std::string>& args)
   }
 
   // Open the volume
-  std::shared_ptr<DiscIO::VolumeDisc> volume = DiscIO::CreateDisc(input_file_path);
+  const std::unique_ptr<DiscIO::VolumeDisc> volume = DiscIO::CreateDisc(input_file_path);
   if (!volume)
   {
     std::cerr << "Error: Unable to open disc image" << std::endl;
@@ -88,26 +84,28 @@ int VerifyCommand::Main(const std::vector<std::string>& args)
   }
 
   // Verify the volume
-  const std::optional<DiscIO::VolumeVerifier::Result> result =
+  DiscIO::VolumeVerifier verifier(*volume, false, hashes_to_calculate);
-      VerifyVolume(volume, hashes_to_calculate);
+  verifier.Start();
-  if (!result)
+  while (verifier.GetBytesProcessed() != verifier.GetTotalBytes())
   {
-    std::cerr << "Error: Unable to verify volume" << std::endl;
+    verifier.Process();
-    return 1;
   }
+  verifier.Finish();
+  const DiscIO::VolumeVerifier::Result& result = verifier.GetResult();
 
-  if (algorithm == std::nullopt)
+  // Print the report
+  if (!algorithm_is_set)
   {
     PrintFullReport(result);
   }
   else
   {
-    if (hashes_to_calculate.crc32 && !result->hashes.crc32.empty())
+    if (hashes_to_calculate.crc32 && !result.hashes.crc32.empty())
-      std::cout << HashToHexString(result->hashes.crc32) << std::endl;
+      std::cout << HashToHexString(result.hashes.crc32) << std::endl;
-    else if (hashes_to_calculate.md5 && !result->hashes.md5.empty())
+    else if (hashes_to_calculate.md5 && !result.hashes.md5.empty())
-      std::cout << HashToHexString(result->hashes.md5) << std::endl;
+      std::cout << HashToHexString(result.hashes.md5) << std::endl;
-    else if (hashes_to_calculate.sha1 && !result->hashes.sha1.empty())
+    else if (hashes_to_calculate.sha1 && !result.hashes.sha1.empty())
-      std::cout << HashToHexString(result->hashes.sha1) << std::endl;
+      std::cout << HashToHexString(result.hashes.sha1) << std::endl;
     else
     {
       std::cerr << "Error: No hash computed" << std::endl;
@@ -118,29 +116,27 @@ int VerifyCommand::Main(const std::vector<std::string>& args)
   return 0;
 }
 
-void VerifyCommand::PrintFullReport(const std::optional<DiscIO::VolumeVerifier::Result> result)
+void VerifyCommand::PrintFullReport(const DiscIO::VolumeVerifier::Result& result)
 {
-  if (!result->hashes.crc32.empty())
+  if (!result.hashes.crc32.empty())
-    std::cout << "CRC32: " << HashToHexString(result->hashes.crc32) << std::endl;
+    std::cout << "CRC32: " << HashToHexString(result.hashes.crc32) << std::endl;
   else
     std::cout << "CRC32 not computed" << std::endl;
 
-  if (!result->hashes.md5.empty())
+  if (!result.hashes.md5.empty())
-    std::cout << "MD5: " << HashToHexString(result->hashes.md5) << std::endl;
+    std::cout << "MD5: " << HashToHexString(result.hashes.md5) << std::endl;
   else
     std::cout << "MD5 not computed" << std::endl;
 
-  if (!result->hashes.sha1.empty())
+  if (!result.hashes.sha1.empty())
-    std::cout << "SHA1: " << HashToHexString(result->hashes.sha1) << std::endl;
+    std::cout << "SHA1: " << HashToHexString(result.hashes.sha1) << std::endl;
   else
     std::cout << "SHA1 not computed" << std::endl;
 
-  std::cout << "Problems Found: " << (result->problems.size() > 0 ? "Yes" : "No") << std::endl;
+  std::cout << "Problems Found: " << (result.problems.empty() ? "No" : "Yes") << std::endl;
 
-  for (int i = 0; i < static_cast<int>(result->problems.size()); ++i)
+  for (const auto& problem : result.problems)
   {
-    const DiscIO::VolumeVerifier::Problem problem = result->problems[i];
-
     std::cout << std::endl << "Severity: ";
     switch (problem.severity)
     {
@@ -166,27 +162,6 @@ void VerifyCommand::PrintFullReport(const std::optional<DiscIO::VolumeVerifier::
   }
 }
 
-std::optional<DiscIO::VolumeVerifier::Result>
-VerifyCommand::VerifyVolume(std::shared_ptr<DiscIO::VolumeDisc> volume,
-                            const DiscIO::Hashes<bool>& hashes_to_calculate)
-{
-  if (!volume)
-    return std::nullopt;
-
-  DiscIO::VolumeVerifier verifier(*volume, false, hashes_to_calculate);
-
-  verifier.Start();
-  while (verifier.GetBytesProcessed() != verifier.GetTotalBytes())
-  {
-    verifier.Process();
-  }
-  verifier.Finish();
-
-  const DiscIO::VolumeVerifier::Result result = verifier.GetResult();
-
-  return result;
-}
-
 std::string VerifyCommand::HashToHexString(const std::vector<u8>& hash)
 {
   std::stringstream ss;
@@ -197,5 +172,4 @@ std::string VerifyCommand::HashToHexString(const std::vector<u8>& hash)
   }
   return ss.str();
 }
-
 }  // namespace DolphinTool

Source/Core/DolphinTool/VerifyCommand.h

@@ -20,11 +20,7 @@ public:
   int Main(const std::vector<std::string>& args) override;
 
 private:
-  void PrintFullReport(const std::optional<DiscIO::VolumeVerifier::Result> result);
+  void PrintFullReport(const DiscIO::VolumeVerifier::Result& result);
-
-  std::optional<DiscIO::VolumeVerifier::Result>
-  VerifyVolume(std::shared_ptr<DiscIO::VolumeDisc> volume,
-               const DiscIO::Hashes<bool>& hashes_to_calculate);
 
   std::string HashToHexString(const std::vector<u8>& hash);
 };