From 7b0565e051140d17e3fb9f4ba4886a9fa1cd561d Mon Sep 17 00:00:00 2001 From: Sushan Jiang Date: Tue, 30 Sep 2025 15:46:16 -0700 Subject: [PATCH] clang-tidy libredex M (#962) Summary: Pull Request resolved: https://github.com/facebook/redex/pull/962 Differential Revision: D83311270 --- libredex/Match.cpp | 17 ++++- libredex/Match.h | 3 - libredex/MatchFlow.h | 4 +- libredex/MethodOverrideGraph.cpp | 6 +- libredex/MethodOverrideGraph.h | 7 +- libredex/MethodProfiles.cpp | 76 ++++++++++--------- libredex/MethodProfiles.h | 6 +- ...dSimilarityCompressionConsciousOrderer.cpp | 6 +- libredex/ReachableClasses.h | 5 ++ 9 files changed, 78 insertions(+), 52 deletions(-) diff --git a/libredex/Match.cpp b/libredex/Match.cpp index 0e26091178c..0e2f54d7572 100644 --- a/libredex/Match.cpp +++ b/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; } diff --git a/libredex/Match.h b/libredex/Match.h index 8a9127a588f..7edfc9ca8b7 100644 --- a/libredex/Match.h +++ b/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" diff --git a/libredex/MatchFlow.h b/libredex/MatchFlow.h index 806e86cad09..b871fc2b4e0 100644 --- a/libredex/MatchFlow.h +++ b/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 diff --git a/libredex/MethodOverrideGraph.cpp b/libredex/MethodOverrideGraph.cpp index f4af970cf03..1131b7f8498 100644 --- a/libredex/MethodOverrideGraph.cpp +++ b/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 gw( [&](std::ostream& os, const DexMethod* method) { const auto& s = show_deobfuscated(method); - bs::write(os, s.size()); + bs::write(os, static_cast(s.size())); os << s; }, [&](const DexMethod* method) -> std::vector { diff --git a/libredex/MethodOverrideGraph.h b/libredex/MethodOverrideGraph.h index 2204aeace2d..aaea583ff5e 100644 --- a/libredex/MethodOverrideGraph.h +++ b/libredex/MethodOverrideGraph.h @@ -91,7 +91,7 @@ struct Node { std::unique_ptr 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 m_nodes; }; diff --git a/libredex/MethodProfiles.cpp b/libredex/MethodProfiles.cpp index 4417de07228..290fa7ae68d 100644 --- a/libredex/MethodProfiles.cpp +++ b/libredex/MethodProfiles.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -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 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 +template 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::max()); - always_assert(parsed >= std::numeric_limits::min()); - return static_cast(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(cell); + auto parsed = parse_int(cell); + always_assert(parsed <= std::numeric_limits::max()); + always_assert(parsed >= 0); + interaction_count = static_cast(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 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::get_method("Lcom/facebook/common/methodpreloader/primarydeps/" - "StartColdStartMethodPreloaderMethodMarker;" - ".startColdStartMethods:()V")); - m_coldstart_end_marker = static_cast( - 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(m_interactions.size()) + + range_begin + score * RANGE_SIZE / 100.0; } continue; } diff --git a/libredex/MethodProfiles.h b/libredex/MethodProfiles.h index 1f438cc2068..90a9c84f7a4 100644 --- a/libredex/MethodProfiles.h +++ b/libredex/MethodProfiles.h @@ -65,7 +65,7 @@ struct Stats { using StatsMap = UnorderedMap; using AllInteractions = std::map; -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& 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 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); }; diff --git a/libredex/MethodSimilarityCompressionConsciousOrderer.cpp b/libredex/MethodSimilarityCompressionConsciousOrderer.cpp index 957f40d4ea4..eef2d082732 100644 --- a/libredex/MethodSimilarityCompressionConsciousOrderer.cpp +++ b/libredex/MethodSimilarityCompressionConsciousOrderer.cpp @@ -84,11 +84,11 @@ void init_bipartite_graph(std::vector& functions, if (freq <= 1) { continue; } - if (static_cast(freq * 2) >= functions.size()) { + if (static_cast(freq) * 2 >= functions.size()) { continue; } - uint32_t new_idx = kmer_index.size(); + uint32_t new_idx = static_cast(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(output.get())); always_assert_log(size <= METHOD_MAX_OUTPUT_SIZE, "Encoded code size limit exceeded %zu versus %zu", size, METHOD_MAX_OUTPUT_SIZE); diff --git a/libredex/ReachableClasses.h b/libredex/ReachableClasses.h index c49fe71876c..4241001cf80 100644 --- a/libredex/ReachableClasses.h +++ b/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 +inline bool has_keep(DexMember* member) { + return member->rstate.has_keep(); +} + inline bool is_serde(const DexClass* member) { return member->rstate.is_serde(); }