From 0ce8d671035f4ecfa6806dbc3948863b27a8a2a6 Mon Sep 17 00:00:00 2001 From: Jendrik Seipp Date: Tue, 9 Jan 2024 12:04:25 +0100 Subject: [PATCH] [issue1129] Enable more performance-related clang-tidy checks and fix code accordingly. --- misc/style/run-clang-tidy.py | 35 +++++++++++++++---- .../cartesian_abstractions/cost_saturation.cc | 2 +- .../cartesian_abstractions/cost_saturation.h | 2 +- src/search/heuristics/lm_cut_landmarks.cc | 4 +-- src/search/heuristics/lm_cut_landmarks.h | 4 +-- src/search/lp/lp_solver.cc | 2 +- src/search/lp/lp_solver.h | 2 +- .../merge_and_shrink_algorithm.cc | 2 +- src/search/parser/abstract_syntax_tree.cc | 2 +- src/search/parser/abstract_syntax_tree.h | 2 +- .../parser/decorated_abstract_syntax_tree.cc | 4 +-- .../parser/decorated_abstract_syntax_tree.h | 4 +-- src/search/task_utils/sampling.cc | 2 +- .../task_utils/variable_order_finder.cc | 2 +- src/search/task_utils/variable_order_finder.h | 2 +- 15 files changed, 46 insertions(+), 25 deletions(-) diff --git a/misc/style/run-clang-tidy.py b/misc/style/run-clang-tidy.py index 5abd781d63..aaaebeefbd 100755 --- a/misc/style/run-clang-tidy.py +++ b/misc/style/run-clang-tidy.py @@ -14,9 +14,11 @@ import utils +LIGHTWEIGHT_TYPES = ["StateID"] IGNORES = [ "'cplex.h' file not found [clang-diagnostic-error]", "'soplex.h' file not found [clang-diagnostic-error]", + "local copy 'copied_key' of the variable 'key' is never modified; consider avoiding the copy [performance-unnecessary-copy-initialization]", ] @@ -47,15 +49,29 @@ def check_search_code_with_clang_tidy(): # categories instead of deleting them to see which additional checks # we could activate. checks = [ - # Enable with CheckTriviallyCopyableMove=0 when we require - # clang-tidy >= 6.0 (see issue856). - # "misc-move-const-arg", - "misc-move-constructor-init", - "misc-use-after-move", + "bugprone-use-after-move", + # "performance-avoid-endl", + # "performance-enum-size", + "performance-faster-string-find", "performance-for-range-copy", - "performance-implicit-cast-in-loop", + "performance-implicit-conversion-in-loop", + "performance-inefficient-algorithm", + # "performance-inefficient-string-concatenation", "performance-inefficient-vector-operation", + # Enable with CheckTriviallyCopyableMove=0 when we require + # clang-tidy >= 6.0 (see issue856). + # "performance-move-const-arg", + #"performance-move-constructor-init", + "performance-no-automatic-move", + # "performance-no-int-to-ptr", + # "performance-noexcept-destructor", + # "performance-noexcept-move-constructor", + # "performance-noexcept-swap", + "performance-trivially-destructible", + "performance-type-promotion-in-math-fn", + "performance-unnecessary-copy-initialization", + "performance-unnecessary-value-param", "readability-avoid-const-params-in-decls", # "readability-braces-around-statements", @@ -83,12 +99,17 @@ def check_search_code_with_clang_tidy(): "readability-static-definition-in-anonymous-namespace", "readability-uniqueptr-delete-release", ] + config = f"""{{CheckOptions: [\ + {{key: performance-unnecessary-value-param.AllowedTypes, value: "{';'.join(LIGHTWEIGHT_TYPES)}"}},\ + ]}}""".replace(" ", "") cmd = [ "run-clang-tidy-12", "-quiet", "-p", build_dir, "-clang-tidy-binary=clang-tidy-12", - "-checks=-*," + ",".join(checks)] + "-checks=-*," + ",".join(checks), + f"-config={config}", + ] print("Running clang-tidy: " + " ".join(pipes.quote(x) for x in cmd)) print() # Don't check returncode here because clang-tidy exits with 1 if it finds any issues. diff --git a/src/search/cartesian_abstractions/cost_saturation.cc b/src/search/cartesian_abstractions/cost_saturation.cc index fda6279544..50fc870a89 100644 --- a/src/search/cartesian_abstractions/cost_saturation.cc +++ b/src/search/cartesian_abstractions/cost_saturation.cc @@ -190,7 +190,7 @@ bool CostSaturation::state_is_dead_end(const State &state) const { void CostSaturation::build_abstractions( const vector> &subtasks, const utils::CountdownTimer &timer, - function should_abort) { + const function &should_abort) { int rem_subtasks = subtasks.size(); for (shared_ptr subtask : subtasks) { subtask = get_remaining_costs_task(subtask); diff --git a/src/search/cartesian_abstractions/cost_saturation.h b/src/search/cartesian_abstractions/cost_saturation.h index e330517a65..93f6629633 100644 --- a/src/search/cartesian_abstractions/cost_saturation.h +++ b/src/search/cartesian_abstractions/cost_saturation.h @@ -49,7 +49,7 @@ class CostSaturation { void build_abstractions( const std::vector> &subtasks, const utils::CountdownTimer &timer, - std::function should_abort); + const std::function &should_abort); void print_statistics(utils::Duration init_time) const; public: diff --git a/src/search/heuristics/lm_cut_landmarks.cc b/src/search/heuristics/lm_cut_landmarks.cc index 71c7602a8d..8fe81d1bb9 100644 --- a/src/search/heuristics/lm_cut_landmarks.cc +++ b/src/search/heuristics/lm_cut_landmarks.cc @@ -274,8 +274,8 @@ void LandmarkCutLandmarks::validate_h_max() const { } bool LandmarkCutLandmarks::compute_landmarks( - const State &state, CostCallback cost_callback, - LandmarkCallback landmark_callback) { + const State &state, const CostCallback &cost_callback, + const LandmarkCallback &landmark_callback) { for (RelaxedOperator &op : relaxed_operators) { op.cost = op.base_cost; } diff --git a/src/search/heuristics/lm_cut_landmarks.h b/src/search/heuristics/lm_cut_landmarks.h index 04b05935fc..a658876d45 100644 --- a/src/search/heuristics/lm_cut_landmarks.h +++ b/src/search/heuristics/lm_cut_landmarks.h @@ -103,8 +103,8 @@ class LandmarkCutLandmarks { Returns true iff state is detected as a dead end. */ - bool compute_landmarks(const State &state, CostCallback cost_callback, - LandmarkCallback landmark_callback); + bool compute_landmarks(const State &state, const CostCallback &cost_callback, + const LandmarkCallback &landmark_callback); }; inline void RelaxedOperator::update_h_max_supporter() { diff --git a/src/search/lp/lp_solver.cc b/src/search/lp/lp_solver.cc index 9a441f0059..9f34d3d520 100644 --- a/src/search/lp/lp_solver.cc +++ b/src/search/lp/lp_solver.cc @@ -107,7 +107,7 @@ const string &LinearProgram::get_objective_name() const { return objective_name; } -void LinearProgram::set_objective_name(string name) { +void LinearProgram::set_objective_name(const string &name) { objective_name = name; } diff --git a/src/search/lp/lp_solver.h b/src/search/lp/lp_solver.h index 401ce1ffa2..a9bdff8c0a 100644 --- a/src/search/lp/lp_solver.h +++ b/src/search/lp/lp_solver.h @@ -91,7 +91,7 @@ class LinearProgram { const named_vector::NamedVector &get_constraints() const; double get_infinity() const; LPObjectiveSense get_sense() const; - void set_objective_name(std::string name); + void set_objective_name(const std::string &name); const std::string &get_objective_name() const; }; diff --git a/src/search/merge_and_shrink/merge_and_shrink_algorithm.cc b/src/search/merge_and_shrink/merge_and_shrink_algorithm.cc index 3834e2d6a1..c1037f7df5 100644 --- a/src/search/merge_and_shrink/merge_and_shrink_algorithm.cc +++ b/src/search/merge_and_shrink/merge_and_shrink_algorithm.cc @@ -33,7 +33,7 @@ using plugins::Bounds; using utils::ExitCode; namespace merge_and_shrink { -static void log_progress(const utils::Timer &timer, string msg, utils::LogProxy &log) { +static void log_progress(const utils::Timer &timer, const string &msg, utils::LogProxy &log) { log << "M&S algorithm timer: " << timer << " (" << msg << ")" << endl; } diff --git a/src/search/parser/abstract_syntax_tree.cc b/src/search/parser/abstract_syntax_tree.cc index 49c1635a93..a8656ab628 100644 --- a/src/search/parser/abstract_syntax_tree.cc +++ b/src/search/parser/abstract_syntax_tree.cc @@ -403,7 +403,7 @@ const plugins::Type &ListNode::get_type(DecorateContext &context) const { } } -LiteralNode::LiteralNode(Token value) +LiteralNode::LiteralNode(const Token &value) : value(value) { } diff --git a/src/search/parser/abstract_syntax_tree.h b/src/search/parser/abstract_syntax_tree.h index 968c7e2c5b..97b35c90f0 100644 --- a/src/search/parser/abstract_syntax_tree.h +++ b/src/search/parser/abstract_syntax_tree.h @@ -89,7 +89,7 @@ class ListNode : public ASTNode { class LiteralNode : public ASTNode { Token value; public: - explicit LiteralNode(Token value); + explicit LiteralNode(const Token &value); DecoratedASTNodePtr decorate(DecorateContext &context) const override; void dump(std::string indent) const override; const plugins::Type &get_type(DecorateContext &context) const override; diff --git a/src/search/parser/decorated_abstract_syntax_tree.cc b/src/search/parser/decorated_abstract_syntax_tree.cc index c1f6294383..d43c49d108 100644 --- a/src/search/parser/decorated_abstract_syntax_tree.cc +++ b/src/search/parser/decorated_abstract_syntax_tree.cc @@ -84,7 +84,7 @@ const DecoratedASTNode &FunctionArgument::get_value() const { return *value; } -void FunctionArgument::dump(string indent) const { +void FunctionArgument::dump(const string &indent) const { cout << indent << key << " = " << endl; value->dump("| " + indent); } @@ -128,7 +128,7 @@ void DecoratedLetNode::dump(string indent) const { } DecoratedFunctionCallNode::DecoratedFunctionCallNode( - shared_ptr feature, vector &&arguments, + const shared_ptr &feature, vector &&arguments, const string &unparsed_config) : feature(feature), arguments(move(arguments)), unparsed_config(unparsed_config) { } diff --git a/src/search/parser/decorated_abstract_syntax_tree.h b/src/search/parser/decorated_abstract_syntax_tree.h index 105f77bf13..d7304213da 100644 --- a/src/search/parser/decorated_abstract_syntax_tree.h +++ b/src/search/parser/decorated_abstract_syntax_tree.h @@ -67,7 +67,7 @@ class FunctionArgument { std::string get_key() const; const DecoratedASTNode &get_value() const; - void dump(std::string indent) const; + void dump(const std::string &indent) const; // TODO: This is here only for the iterated search. Once we switch to builders, we won't need it any more. bool is_lazily_constructed() const; @@ -99,7 +99,7 @@ class DecoratedFunctionCallNode : public DecoratedASTNode { std::string unparsed_config; public: DecoratedFunctionCallNode( - std::shared_ptr feature, + const std::shared_ptr &feature, std::vector &&arguments, const std::string &unparsed_config); diff --git a/src/search/task_utils/sampling.cc b/src/search/task_utils/sampling.cc index b59c9094a2..86281fc6fd 100644 --- a/src/search/task_utils/sampling.cc +++ b/src/search/task_utils/sampling.cc @@ -19,7 +19,7 @@ static State sample_state_with_random_walk( int init_h, double average_operator_cost, utils::RandomNumberGenerator &rng, - function is_dead_end) { + const function &is_dead_end) { assert(init_h != numeric_limits::max()); int n; if (init_h == 0) { diff --git a/src/search/task_utils/variable_order_finder.cc b/src/search/task_utils/variable_order_finder.cc index 6a741b913d..cd0f8752fa 100644 --- a/src/search/task_utils/variable_order_finder.cc +++ b/src/search/task_utils/variable_order_finder.cc @@ -17,7 +17,7 @@ using utils::ExitCode; namespace variable_order_finder { VariableOrderFinder::VariableOrderFinder(const TaskProxy &task_proxy, VariableOrderType variable_order_type, - shared_ptr rng) + const shared_ptr &rng) : task_proxy(task_proxy), variable_order_type(variable_order_type) { int var_count = task_proxy.get_variables().size(); diff --git a/src/search/task_utils/variable_order_finder.h b/src/search/task_utils/variable_order_finder.h index d6234c06b5..3873f53e66 100644 --- a/src/search/task_utils/variable_order_finder.h +++ b/src/search/task_utils/variable_order_finder.h @@ -42,7 +42,7 @@ class VariableOrderFinder { VariableOrderFinder( const TaskProxy &task_proxy, VariableOrderType variable_order_type, - std::shared_ptr rng = nullptr); + const std::shared_ptr &rng = nullptr); ~VariableOrderFinder() = default; bool done() const; int next();