clang-tidy libredex M

libredex/Match.cpp

@@ -51,8 +51,21 @@ bool is_default_constructor(const DexMethod* meth) {
     auto it = ii.begin();
     const auto end = ii.end();
 
-    return it != end && it++->insn->opcode() == OPCODE_INVOKE_DIRECT &&
-           it != end && it++->insn->opcode() == OPCODE_RETURN_VOID && it == end;
+    if (it == end) {
+      return false;
+    }
+    if (it->insn->opcode() != OPCODE_INVOKE_DIRECT) {
+      return false;
+    }
+    ++it;
+    if (it == end) {
+      return false;
+    }
+    if (it->insn->opcode() != OPCODE_RETURN_VOID) {
+      return false;
+    }
+    ++it;
+    return it == end;
   }
   return false;
 }

libredex/Match.h

@@ -16,10 +16,7 @@
 #include "DexAccess.h"
 #include "DexAnnotation.h"
 #include "DexClass.h"
-#include "DexUtil.h"
-#include "IRCode.h"
 #include "IRInstruction.h"
-#include "MethodUtil.h"
 #include "ReachableClasses.h"
 #include "Resolver.h"
 

libredex/MatchFlow.h

@@ -283,8 +283,8 @@ namespace detail {
 // Re-open the detail namespace to define operator|, because ADL only adds the
 // innermost namespaces for parameter types to the lookup set.
 
-inline constexpr flag_t operator|(AliasFlag a, QuantFlag q) { return {a, q}; }
-inline constexpr flag_t operator|(QuantFlag q, AliasFlag a) { return {a, q}; }
+constexpr flag_t operator|(AliasFlag a, QuantFlag q) { return {a, q}; }
+constexpr flag_t operator|(QuantFlag q, AliasFlag a) { return {a, q}; }
 
 } // namespace detail
 

libredex/MethodOverrideGraph.cpp

@@ -368,8 +368,6 @@ bool all_overridden_methods_impl(const Graph& graph,
 
 namespace method_override_graph {
 
-Node Graph::empty_node;
-
 bool Node::overrides(const DexMethod* current, const DexType* base_type) const {
   // Trivial case.
   if (type::check_cast(current->get_class(), base_type)) {
@@ -392,7 +390,7 @@ bool Node::overrides(const DexMethod* current, const DexType* base_type) const {
 const Node& Graph::get_node(const DexMethod* method) const {
   auto it = m_nodes.find(method);
   if (it == m_nodes.end()) {
-    return empty_node;
+    return get_empty_node();
   }
   return it->second;
 }
@@ -483,7 +481,7 @@ void Graph::dump(std::ostream& os) const {
   bs::GraphWriter<const DexMethod*> gw(
       [&](std::ostream& os, const DexMethod* method) {
         const auto& s = show_deobfuscated(method);
-        bs::write<uint32_t>(os, s.size());
+        bs::write<uint32_t>(os, static_cast<uint32_t>(s.size()));
         os << s;
       },
       [&](const DexMethod* method) -> std::vector<const DexMethod*> {

libredex/MethodOverrideGraph.h

@@ -91,7 +91,7 @@ struct Node {
   std::unique_ptr<OtherInterfaceImplementations>
       other_interface_implementations;
   // Whether the current Node's method is an interface method.
-  bool is_interface;
+  bool is_interface{false};
 
   // Checks whther the current method's class, or any other implementation
   // class, can be cast to the given base type.
@@ -120,7 +120,10 @@ class Graph {
   void dump(std::ostream&) const;
 
  private:
-  static Node empty_node;
+  static const Node& get_empty_node() {
+    static const Node empty_node;
+    return empty_node;
+  }
   ConcurrentMap<const DexMethod*, Node> m_nodes;
 };
 

libredex/MethodProfiles.cpp

@@ -13,6 +13,7 @@
 
 #include <boost/algorithm/string.hpp>
 #include <boost/algorithm/string/regex.hpp>
+#include <charconv>
 #include <fstream>
 #include <iostream>
 #include <stdlib.h>
@@ -49,8 +50,11 @@ bool empty_column(std::string_view sv) { return sv.empty() || sv == "\n"; }
 
 } // namespace
 
-AccumulatingTimer MethodProfiles::s_process_unresolved_lines_timer(
-    "MethodProfiles::process_unresolved_lines");
+AccumulatingTimer& MethodProfiles::get_process_unresolved_lines_timer() {
+  static AccumulatingTimer s_process_unresolved_lines_timer(
+      "MethodProfiles::process_unresolved_lines");
+  return s_process_unresolved_lines_timer;
+}
 
 std::tuple<const StatsMap&, bool> method_stats_for_interaction_id(
     const std::string& interaction_id, const AllInteractions& interactions) {
@@ -369,7 +373,7 @@ bool MethodProfiles::parse_stats_file(const std::string& csv_filename,
   // iostreams equivalent and we expect to read very large csv files.
   std::ifstream ifs(csv_filename);
   if (!ifs.good()) {
-    std::cerr << "FAILED to open " << csv_filename << std::endl;
+    std::cerr << "FAILED to open " << csv_filename << '\n';
     return false;
   }
 
@@ -395,7 +399,7 @@ bool MethodProfiles::parse_stats_file(const std::string& csv_filename,
     }
   }
   if (ifs.bad()) {
-    std::cerr << "FAILED to read a line!" << std::endl;
+    std::cerr << "FAILED to read a line!" << '\n';
     return false;
   }
 
@@ -405,28 +409,25 @@ bool MethodProfiles::parse_stats_file(const std::string& csv_filename,
   return true;
 }
 
-// `strtol` and `strtod` requires c string to be null terminated,
-// std::string_view::data() doesn't have this guarantee. Our `string_view`s are
-// taken from `std::string`s. This should be safe.
-template <typename IntType>
+template <typename IntType = int64_t>
 IntType parse_int(std::string_view tok) {
-  char* ptr = nullptr;
-  const auto parsed = strtol(tok.data(), &ptr, 10);
-  always_assert_log(
-      ptr <= (tok.data() + tok.size()),
-      "strtod went over std::string_view boundary for string_view %s",
-      SHOW(tok));
-
+  IntType result{};
+  auto [ptr, ec]{std::from_chars(tok.data(), tok.data() + tok.size(), result)};
   std::string_view rest(ptr, tok.size() - (ptr - tok.data()));
-  always_assert_log(ptr != tok.data(), "can't parse %s into a int", SHOW(tok));
+
+  always_assert_log(ec == std::errc(), "can't parse %s into a int: %s",
+                    SHOW(tok), std::make_error_condition(ec).message().c_str());
   always_assert_log(empty_column(rest), "can't parse %s into a int", SHOW(tok));
-  always_assert(parsed <= std::numeric_limits<IntType>::max());
-  always_assert(parsed >= std::numeric_limits<IntType>::min());
-  return static_cast<IntType>(parsed);
+  return result;
 }
 
+// `strtol` and `strtod` requires c string to be null terminated,
+// std::string_view::data() doesn't have this guarantee. Our `string_view`s are
+// taken from `std::string`s. This should be safe.
+// Combine with parse_int above after we drop some older compiler support.
 double parse_double(std::string_view tok) {
   char* ptr = nullptr;
+  // NOLINTNEXTLINE(bugprone-suspicious-stringview-data-usage)
   const auto result = strtod(tok.data(), &ptr);
   always_assert_log(
       ptr <= (tok.data() + tok.size()),
@@ -450,14 +451,16 @@ bool MethodProfiles::parse_metadata(std::string_view line) {
       m_interaction_id = std::string(cell);
       return true;
     case 1: {
-      interaction_count = parse_int<uint32_t>(cell);
+      auto parsed = parse_int(cell);
+      always_assert(parsed <= std::numeric_limits<uint32_t>::max());
+      always_assert(parsed >= 0);
+      interaction_count = static_cast<uint32_t>(parsed);
       return true;
     }
     default: {
       bool ok = empty_column(cell);
       if (!ok) {
-        std::cerr << "Unexpected extra value in metadata: " << cell
-                  << std::endl;
+        std::cerr << "Unexpected extra value in metadata: " << cell << '\n';
       }
       return ok;
     }
@@ -727,7 +730,7 @@ void MethodProfiles::process_unresolved_lines(bool baseline_profile_variant) {
     return;
   }
 
-  auto timer_scope = s_process_unresolved_lines_timer.scope();
+  auto timer_scope = get_process_unresolved_lines_timer().scope();
 
   std::set<ParsedMain*> resolved;
   std::mutex resolved_mutex;
@@ -858,7 +861,7 @@ bool MethodProfiles::parse_header(std::string_view line) {
       default: {
         auto ok = empty_column(cell);
         if (!ok) {
-          std::cerr << "Unexpected Metadata Column: " << cell << std::endl;
+          std::cerr << "Unexpected Metadata Column: " << cell << '\n';
         }
         return ok;
       }
@@ -907,14 +910,18 @@ dexmethods_profiled_comparator::dexmethods_profiled_comparator(
 
   m_cache.reserve(initial_order.size());
 
-  m_coldstart_start_marker = static_cast<DexMethod*>(
-      DexMethod::get_method("Lcom/facebook/common/methodpreloader/primarydeps/"
-                            "StartColdStartMethodPreloaderMethodMarker;"
-                            ".startColdStartMethods:()V"));
-  m_coldstart_end_marker = static_cast<DexMethod*>(
-      DexMethod::get_method("Lcom/facebook/common/methodpreloader/primarydeps/"
-                            "EndColdStartMethodPreloaderMethodMarker;"
-                            ".endColdStartMethods:()V"));
+  auto* start_ref = DexMethod::get_method(
+      "Lcom/facebook/common/methodpreloader/primarydeps/"
+      "StartColdStartMethodPreloaderMethodMarker;"
+      ".startColdStartMethods:()V");
+  m_coldstart_start_marker =
+      (start_ref != nullptr) ? start_ref->as_def() : nullptr;
+
+  auto* end_ref = DexMethod::get_method(
+      "Lcom/facebook/common/methodpreloader/primarydeps/"
+      "EndColdStartMethodPreloaderMethodMarker;"
+      ".endColdStartMethods:()V");
+  m_coldstart_end_marker = (end_ref != nullptr) ? end_ref->as_def() : nullptr;
 
   for (const auto& pair : m_method_profiles->all_interactions()) {
     std::string interaction_id = pair.first;
@@ -1002,8 +1009,9 @@ double dexmethods_profiled_comparator::get_method_sort_num(
               mixed_ordering(stat.appear_percent, m_min_appear_percent,
                              stat.order_percent, 0.1);
 
-          secondary_ordering = RANGE_STRIDE * m_interactions.size() +
-                               range_begin + score * RANGE_SIZE / 100.0;
+          secondary_ordering =
+              RANGE_STRIDE * static_cast<double>(m_interactions.size()) +
+              range_begin + score * RANGE_SIZE / 100.0;
         }
         continue;
       }

libredex/MethodProfiles.h

@@ -65,7 +65,7 @@ struct Stats {
 
 using StatsMap = UnorderedMap<const DexMethodRef*, Stats>;
 using AllInteractions = std::map<std::string, StatsMap>;
-const std::string COLD_START = "ColdStart";
+inline const std::string COLD_START = "ColdStart";
 
 class MethodProfiles {
  public:
@@ -200,7 +200,7 @@ class MethodProfiles {
                           const std::vector<DexMethod*>& sources);
 
  private:
-  static AccumulatingTimer s_process_unresolved_lines_timer;
+  static AccumulatingTimer& get_process_unresolved_lines_timer();
   AllInteractions m_method_stats;
   AllInteractions m_baseline_profile_method_stats;
   std::map<std::string, AllInteractions*> m_baseline_manual_interactions;
@@ -317,6 +317,8 @@ class dexmethods_profiled_comparator {
   // See class comment.
   dexmethods_profiled_comparator(const dexmethods_profiled_comparator&) =
       delete;
+  dexmethods_profiled_comparator& operator=(
+      const dexmethods_profiled_comparator&) = delete;
 
   bool operator()(DexMethod* a, DexMethod* b);
 };

libredex/MethodSimilarityCompressionConsciousOrderer.cpp

@@ -84,11 +84,11 @@ void init_bipartite_graph(std::vector<BinaryFunction>& functions,
     if (freq <= 1) {
       continue;
     }
-    if (static_cast<size_t>(freq * 2) >= functions.size()) {
+    if (static_cast<size_t>(freq) * 2 >= functions.size()) {
       continue;
     }
 
-    uint32_t new_idx = kmer_index.size();
+    uint32_t new_idx = static_cast<uint32_t>(kmer_index.size());
     kmer_index[kmer] = new_idx;
   }
 
@@ -217,7 +217,7 @@ MethodSimilarityCompressionConsciousOrderer::get_encoded_method_content(
   memset(output.get(), 0, METHOD_MAX_OUTPUT_SIZE);
 
   // Encode
-  size_t size = code->encode(&dodx, (uint32_t*)(output.get()));
+  size_t size = code->encode(&dodx, reinterpret_cast<uint32_t*>(output.get()));
   always_assert_log(size <= METHOD_MAX_OUTPUT_SIZE,
                     "Encoded code size limit exceeded %zu versus %zu", size,
                     METHOD_MAX_OUTPUT_SIZE);

libredex/ReachableClasses.h

@@ -67,6 +67,11 @@ inline bool can_rename_if_also_renaming_xml(DexMember* member) {
   return member->rstate.can_rename_if_also_renaming_xml();
 }
 
+template <class DexMember>
+inline bool has_keep(DexMember* member) {
+  return member->rstate.has_keep();
+}
+
 inline bool is_serde(const DexClass* member) {
   return member->rstate.is_serde();
 }