From 441a3b1be1588a77c4642d7d54393ca1c4b8a8e6 Mon Sep 17 00:00:00 2001 From: Ioannis Magkanaris Date: Mon, 18 Sep 2023 11:31:19 +0200 Subject: [PATCH] Fix codegen_cpp_visitor.*pp based on cppcheck (#1073) * Trying to fix various aspects of codegen_cpp_visitor.*pp based on suggestions from cppcheck * Add vscode specific file to ignored files * Improved time and date printing * Renaming of arguments * Improve CodegenCppVisitor constructors * Use std::any_of with lamda instead of for loops --- .gitignore | 1 + src/codegen/codegen_cpp_visitor.cpp | 71 +++++++-------- src/codegen/codegen_cpp_visitor.hpp | 132 +++++++++++----------------- 3 files changed, 86 insertions(+), 118 deletions(-) diff --git a/.gitignore b/.gitignore index 3fe72144cb..ffdb3ea734 100644 --- a/.gitignore +++ b/.gitignore @@ -42,6 +42,7 @@ venv/ ENV/ env.bak/ venv.bak/ +.vscode # HPC coding conventions .clang-format diff --git a/src/codegen/codegen_cpp_visitor.cpp b/src/codegen/codegen_cpp_visitor.cpp index 450139cd82..88fd8985dc 100644 --- a/src/codegen/codegen_cpp_visitor.cpp +++ b/src/codegen/codegen_cpp_visitor.cpp @@ -8,6 +8,7 @@ #include "codegen/codegen_cpp_visitor.hpp" #include +#include #include #include #include @@ -1433,9 +1434,9 @@ void CodegenCppVisitor::print_table_check_function(const Block& node) { } for (const auto& variable: depend_variables) { - auto name = variable->get_node_name(); - auto instance_name = get_variable_name(name); - printer->fmt_start_block("if (save_{} != {})", name, instance_name); + auto var_name = variable->get_node_name(); + auto instance_name = get_variable_name(var_name); + printer->fmt_start_block("if (save_{} != {})", var_name, instance_name); printer->add_line("make_table = true;"); printer->end_block(1); } @@ -1466,10 +1467,10 @@ void CodegenCppVisitor::print_table_check_function(const Block& node) { if (node.is_procedure_block()) { printer->fmt_line("{}({}, x);", function, internal_method_arguments()); for (const auto& variable: table_variables) { - auto name = variable->get_node_name(); - auto instance_name = get_variable_name(name); - auto table_name = get_variable_name("t_" + name); - auto [is_array, array_length] = check_if_var_is_array(name); + auto var_name = variable->get_node_name(); + auto instance_name = get_variable_name(var_name); + auto table_name = get_variable_name("t_" + var_name); + auto [is_array, array_length] = check_if_var_is_array(var_name); if (is_array) { for (int j = 0; j < array_length; j++) { printer->fmt_line( @@ -1489,9 +1490,9 @@ void CodegenCppVisitor::print_table_check_function(const Block& node) { printer->end_block(1); for (const auto& variable: depend_variables) { - auto name = variable->get_node_name(); - auto instance_name = get_variable_name(name); - printer->fmt_line("save_{} = {};", name, instance_name); + auto var_name = variable->get_node_name(); + auto instance_name = get_variable_name(var_name); + printer->fmt_line("save_{} = {};", var_name, instance_name); } } printer->end_block(1); @@ -1506,7 +1507,6 @@ void CodegenCppVisitor::print_table_replacement_function(const ast::Block& node) auto table_variables = statement->get_table_vars(); auto with = statement->get_with()->eval(); auto use_table_var = get_variable_name(naming::USE_TABLE_VARIABLE); - auto float_type = default_float_data_type(); auto tmin_name = get_variable_name("tmin_" + name); auto mfac_name = get_variable_name("mfac_" + name); auto function_name = method_name("f_" + name); @@ -1538,14 +1538,14 @@ void CodegenCppVisitor::print_table_replacement_function(const ast::Block& node) printer->start_block("if (isnan(xi))"); if (node.is_procedure_block()) { for (const auto& var: table_variables) { - auto name = get_variable_name(var->get_node_name()); + auto var_name = get_variable_name(var->get_node_name()); auto [is_array, array_length] = check_if_var_is_array(var->get_node_name()); if (is_array) { for (int j = 0; j < array_length; j++) { - printer->fmt_line("{}[{}] = xi;", name, j); + printer->fmt_line("{}[{}] = xi;", var_name, j); } } else { - printer->fmt_line("{} = xi;", name); + printer->fmt_line("{} = xi;", var_name); } } printer->add_line("return 0;"); @@ -1558,10 +1558,10 @@ void CodegenCppVisitor::print_table_replacement_function(const ast::Block& node) printer->fmt_line("int index = (xi <= 0.) ? 0 : {};", with); if (node.is_procedure_block()) { for (const auto& variable: table_variables) { - auto name = variable->get_node_name(); - auto instance_name = get_variable_name(name); - auto table_name = get_variable_name("t_" + name); - auto [is_array, array_length] = check_if_var_is_array(name); + auto var_name = variable->get_node_name(); + auto instance_name = get_variable_name(var_name); + auto table_name = get_variable_name("t_" + var_name); + auto [is_array, array_length] = check_if_var_is_array(var_name); if (is_array) { for (int j = 0; j < array_length; j++) { printer->fmt_line( @@ -1582,9 +1582,9 @@ void CodegenCppVisitor::print_table_replacement_function(const ast::Block& node) printer->add_line("double theta = xi - double(i);"); if (node.is_procedure_block()) { for (const auto& var: table_variables) { - auto name = var->get_node_name(); - auto instance_name = get_variable_name(name); - auto table_name = get_variable_name("t_" + name); + auto var_name = var->get_node_name(); + auto instance_name = get_variable_name(var_name); + auto table_name = get_variable_name("t_" + var_name); auto [is_array, array_length] = check_if_var_is_array(var->get_node_name()); if (is_array) { for (size_t j = 0; j < array_length; j++) { @@ -1625,9 +1625,9 @@ void CodegenCppVisitor::print_check_table_thread_function() { printer->add_line("double v = 0;"); for (const auto& function: info.functions_with_table) { - auto name = method_name("check_" + function->get_node_name()); + auto method_name_str = method_name("check_" + function->get_node_name()); auto arguments = internal_method_arguments(); - printer->fmt_line("{}({});", name, arguments); + printer->fmt_line("{}({});", method_name_str, arguments); } printer->end_block(1); @@ -1736,9 +1736,6 @@ void CodegenCppVisitor::print_function_tables(const ast::FunctionTableBlock& nod */ bool is_functor_const(const ast::StatementBlock& variable_block, const ast::StatementBlock& functor_block) { - // Save DUChain for every variable in variable_block - std::unordered_map chains; - // Create complete_block with both variable declarations (done in variable_block) and solver // part (done in functor_block) to be able to run the SymtabVisitor and DefUseAnalyzeVisitor // then and get the proper DUChains for the variables defined in the variable_block @@ -2416,9 +2413,9 @@ std::string CodegenCppVisitor::get_variable_name(const std::string& name, bool u void CodegenCppVisitor::print_backend_info() { - time_t tr{}; - time(&tr); - auto date = std::string(asctime(localtime(&tr))); + time_t current_time{}; + time(¤t_time); + std::string data_time_str{std::ctime(¤t_time)}; auto version = nmodl::Version::NMODL_VERSION + " [" + nmodl::Version::GIT_REVISION + "]"; printer->add_line("/*********************************************************"); @@ -2427,7 +2424,7 @@ void CodegenCppVisitor::print_backend_info() { printer->fmt_line("NMODL Version : {}", nmodl_version()); printer->fmt_line("Vectorized : {}", info.vectorize); printer->fmt_line("Threadsafe : {}", info.thread_safe); - printer->fmt_line("Created : {}", stringutils::trim(date)); + printer->fmt_line("Created : {}", stringutils::trim(data_time_str)); printer->fmt_line("Backend : {}", backend_name()); printer->fmt_line("NMODL Compiler : {}", version); printer->add_line("*********************************************************/"); @@ -2711,7 +2708,7 @@ void CodegenCppVisitor::print_prcellstate_macros() const { void CodegenCppVisitor::print_mechanism_info() { - auto variable_printer = [&](std::vector& variables) { + auto variable_printer = [&](const std::vector& variables) { for (const auto& v: variables) { auto name = v->get_name(); if (!info.point_process) { @@ -2871,18 +2868,18 @@ void CodegenCppVisitor::print_mechanism_register() { printer->add_line("_nrn_layout_reg(mech_type, 0);"); // 0 for SoA // register mechanism - auto args = register_mechanism_arguments(); - auto nobjects = num_thread_objects(); + const auto mech_arguments = register_mechanism_arguments(); + const auto number_of_thread_objects = num_thread_objects(); if (info.point_process) { printer->fmt_line("point_register_mech({}, {}, {}, {});", - args, + mech_arguments, info.constructor_node ? method_name(naming::NRN_CONSTRUCTOR_METHOD) : "nullptr", info.destructor_node ? method_name(naming::NRN_DESTRUCTOR_METHOD) : "nullptr", - nobjects); + number_of_thread_objects); } else { - printer->fmt_line("register_mech({}, {});", args, nobjects); + printer->fmt_line("register_mech({}, {});", mech_arguments, number_of_thread_objects); if (info.constructor_node) { printer->fmt_line("register_constructor({});", method_name(naming::NRN_CONSTRUCTOR_METHOD)); @@ -3059,7 +3056,6 @@ void CodegenCppVisitor::print_thread_memory_callbacks() { void CodegenCppVisitor::print_mechanism_range_var_structure(bool print_initialisers) { auto const value_initialise = print_initialisers ? "{}" : ""; - auto float_type = default_float_data_type(); auto int_type = default_int_data_type(); printer->add_newline(2); printer->add_line("/** all mechanism instance variables and global variables */"); @@ -3797,7 +3793,6 @@ void CodegenCppVisitor::print_net_move_call(const FunctionCall& node) { printer->add_text(")"); } else { auto point_process = get_variable_name("point_process"); - std::string t = get_variable_name("t"); printer->add_text("net_send_buffering("); printer->fmt_text("nt, ml->_net_send_buffer, 2, {}, {}, {}, ", tqitem, weight_index, point_process); print_vector_elements(arguments, ", "); diff --git a/src/codegen/codegen_cpp_visitor.hpp b/src/codegen/codegen_cpp_visitor.hpp index 09e6f0bd0a..0cb44081f5 100644 --- a/src/codegen/codegen_cpp_visitor.hpp +++ b/src/codegen/codegen_cpp_visitor.hpp @@ -127,10 +127,10 @@ struct IndexVariableInfo { /// if the variable is qualified as constant (this is property of IndexVariable) bool is_constant = false; - IndexVariableInfo(std::shared_ptr symbol, - bool is_vdata = false, - bool is_index = false, - bool is_integer = false) + explicit IndexVariableInfo(std::shared_ptr symbol, + bool is_vdata = false, + bool is_index = false, + bool is_integer = false) : symbol(std::move(symbol)) , is_vdata(is_vdata) , is_index(is_index) @@ -191,21 +191,41 @@ class CodegenCppVisitor: public visitor::ConstAstVisitor { */ using ParamVector = std::vector>; + /** + * Code printer object for target (C++) + */ + std::shared_ptr target_printer; + + /** + * Code printer object for wrappers + */ + std::shared_ptr wrapper_printer; + + /** + * Pointer to active code printer + */ + std::shared_ptr printer; + /** * Name of mod file (without .mod suffix) */ std::string mod_filename; /** - * Flag to indicate if visitor should print the visited nodes + * Data type of floating point variables */ - bool codegen = false; + std::string float_type = codegen::naming::DEFAULT_FLOAT_TYPE; /** * Flag to indicate if visitor should avoid ion variable copies */ bool optimize_ionvar_copies = true; + /** + * Flag to indicate if visitor should print the visited nodes + */ + bool codegen = false; + /** * Variable name should be converted to instance name (but not for function arguments) */ @@ -258,31 +278,11 @@ class CodegenCppVisitor: public visitor::ConstAstVisitor { */ int current_watch_statement = 0; - /** - * Data type of floating point variables - */ - std::string float_type = codegen::naming::DEFAULT_FLOAT_TYPE; - /** * All ast information for code generation */ codegen::CodegenInfo info; - /** - * Code printer object for target (C) - */ - std::shared_ptr target_printer; - - /** - * Code printer object for wrappers - */ - std::shared_ptr wrapper_printer; - - /** - * Pointer to active code printer - */ - std::shared_ptr printer; - /** * Return Nmodl language version * \return A version @@ -1577,30 +1577,31 @@ class CodegenCppVisitor: public visitor::ConstAstVisitor { virtual void print_wrapper_routines(); - CodegenCppVisitor(const std::string& mod_filename, + CodegenCppVisitor(std::string mod_filename, const std::string& output_dir, - const std::string& float_type, + std::string float_type, const bool optimize_ionvar_copies, const std::string& extension, const std::string& wrapper_ext) - : target_printer(new CodePrinter(output_dir + "/" + mod_filename + extension)) - , wrapper_printer(new CodePrinter(output_dir + "/" + mod_filename + wrapper_ext)) + : target_printer(std::make_shared(output_dir + "/" + mod_filename + extension)) + , wrapper_printer( + std::make_shared(output_dir + "/" + mod_filename + wrapper_ext)) , printer(target_printer) - , mod_filename(mod_filename) - , float_type(float_type) + , mod_filename(std::move(mod_filename)) + , float_type(std::move(float_type)) , optimize_ionvar_copies(optimize_ionvar_copies) {} - CodegenCppVisitor(const std::string& mod_filename, + CodegenCppVisitor(std::string mod_filename, std::ostream& stream, - const std::string& float_type, + std::string float_type, const bool optimize_ionvar_copies, const std::string& extension, const std::string& wrapper_ext) - : target_printer(new CodePrinter(stream)) - , wrapper_printer(new CodePrinter(stream)) + : target_printer(std::make_shared(stream)) + , wrapper_printer(std::make_shared(stream)) , printer(target_printer) - , mod_filename(mod_filename) - , float_type(float_type) + , mod_filename(std::move(mod_filename)) + , float_type(std::move(float_type)) , optimize_ionvar_copies(optimize_ionvar_copies) {} @@ -1622,14 +1623,14 @@ class CodegenCppVisitor: public visitor::ConstAstVisitor { * as-is in the target code. This defaults to \c double. * \param extension The file extension to use. This defaults to \c .cpp . */ - CodegenCppVisitor(const std::string& mod_filename, + CodegenCppVisitor(std::string mod_filename, const std::string& output_dir, std::string float_type, const bool optimize_ionvar_copies, const std::string& extension = ".cpp") - : target_printer(new CodePrinter(output_dir + "/" + mod_filename + extension)) + : target_printer(std::make_shared(output_dir + "/" + mod_filename + extension)) , printer(target_printer) - , mod_filename(mod_filename) + , mod_filename(std::move(mod_filename)) , float_type(std::move(float_type)) , optimize_ionvar_copies(optimize_ionvar_copies) {} @@ -1649,42 +1650,14 @@ class CodegenCppVisitor: public visitor::ConstAstVisitor { * \param float_type The float type to use in the generated code. The string will be used * as-is in the target code. This defaults to \c double. */ - CodegenCppVisitor(const std::string& mod_filename, - std::ostream& stream, - const std::string& float_type, - const bool optimize_ionvar_copies) - : target_printer(new CodePrinter(stream)) - , printer(target_printer) - , mod_filename(mod_filename) - , float_type(float_type) - , optimize_ionvar_copies(optimize_ionvar_copies) {} - - - /** - * \copybrief nmodl::codegen::CodegenCppVisitor - * - * This constructor instantiates an NMODL C code generator and allows writing generated code - * using an nmodl::printer::CodePrinter defined elsewhere. - * - * \note No code generation is performed at this stage. Since the code - * generator classes are all based on \c AstVisitor the AST must be visited using e.g. \c - * visit_program in order to generate the C code corresponding to the AST. - * - * \param mod_filename The name of the model for which code should be generated. - * It is used for constructing an output filename. - * \param float_type The float type to use in the generated code. The string will be used - * as-is in the target code. This defaults to \c double. - * \param target_printer A printer defined outside this visitor to be used for the code - * generation - */ CodegenCppVisitor(std::string mod_filename, + std::ostream& stream, std::string float_type, - const bool optimize_ionvar_copies, - std::shared_ptr& target_printer) - : target_printer(target_printer) + const bool optimize_ionvar_copies) + : target_printer(std::make_shared(stream)) , printer(target_printer) - , mod_filename(mod_filename) - , float_type(float_type) + , mod_filename(std::move(mod_filename)) + , float_type(std::move(float_type)) , optimize_ionvar_copies(optimize_ionvar_copies) {} @@ -1890,12 +1863,11 @@ void CodegenCppVisitor::print_vector_elements(const std::vector& elements, template bool has_parameter_of_name(const T& node, const std::string& name) { auto parameters = node->get_parameters(); - for (const auto& parameter: parameters) { - if (parameter->get_node_name() == name) { - return true; - } - } - return false; + return std::any_of(parameters.begin(), + parameters.end(), + [&name](const decltype(*parameters.begin()) arg) { + return arg->get_node_name() == name; + }); }