make nmodl::stringutils methods more consistent (#1071)

* The prototypes of these functions now follow these 2 principles: 1. the input string is kept intact 2. the transformed text is returned in a new `std::string` instance * Improve the Doxygen documentation Also removed some useless work along the way while updating the calls to these functions. Fixes #1066 Co-authored-by: Luc Grosheintz <[email protected]>
BlueBrain · Sep 8, 2023 · 8af222d · 8af222d
1 parent f62fa77
commit 8af222d
Show file tree

Hide file tree

Showing 9 changed files with 117 additions and 69 deletions.
diff --git a/src/codegen/codegen_cpp_visitor.cpp b/src/codegen/codegen_cpp_visitor.cpp
@@ -302,14 +302,14 @@ void CodegenCppVisitor::visit_verbatim(const Verbatim& node) {
     if (!codegen) {
         return;
     }
-    auto text = node.get_statement()->eval();
-    auto result = process_verbatim_text(text);
-
-    auto statements = stringutils::split_string(result, '\n');
-    for (auto& statement: statements) {
-        stringutils::trim_newline(statement);
-        if (statement.find_first_not_of(' ') != std::string::npos) {
-            printer->add_line(statement);
+    const auto& text = node.get_statement()->eval();
+    const auto& result = process_verbatim_text(text);
+
+    const auto& statements = stringutils::split_string(result, '\n');
+    for (const auto& statement: statements) {
+        const auto& trimed_stmt = stringutils::trim_newline(statement);
+        if (trimed_stmt.find_first_not_of(' ') != std::string::npos) {
+            printer->add_line(trimed_stmt);
         }
     }
 }
@@ -1373,9 +1373,9 @@ void CodegenCppVisitor::print_top_verbatim_blocks() {
  * the text.
  */
 void CodegenCppVisitor::rename_function_arguments() {
-    auto default_arguments = stringutils::split_string(nrn_thread_arguments(), ',');
-    for (auto& arg: default_arguments) {
-        stringutils::trim(arg);
+    const auto& default_arguments = stringutils::split_string(nrn_thread_arguments(), ',');
+    for (const auto& dirty_arg: default_arguments) {
+        const auto& arg = stringutils::trim(dirty_arg);
         RenameVisitor v(arg, "arg_" + arg);
         for (const auto& function: info.functions) {
             if (has_parameter_of_name(function, arg)) {

diff --git a/src/lexer/c11.ll b/src/lexer/c11.ll
@@ -668,7 +668,7 @@ WS  [ \t\v\n\f]
 {WS}+                                   {
                                             driver.add_token(yytext);
                                             std::string str(yytext);
-                                            stringutils::remove_character(str, ' ');
+                                            str = stringutils::remove_character(str, ' ');
                                             if (str == "\n") {
                                                 loc.lines(1);
                                             } else {

diff --git a/src/lexer/nmodl.ll b/src/lexer/nmodl.ll
@@ -392,11 +392,11 @@ ELSE                    {
                               * for using lexer program. */
                             std::string str(yytext);
                             cur_line = str;
-                            stringutils::trim(str);
+                            str = stringutils::trim(str);
 
                             if (driver.is_verbose()) {
                                 if(str.length()) {
-                                    stringutils::trim_newline(str);
+                                    str = stringutils::trim_newline(str);
                                     std::cout << "LINE "<< yylineno << ": " << str << std::endl;
                                 } else {
                                     std::cout << "LINE " << yylineno << ": " << std::endl;
@@ -472,7 +472,7 @@ ELSE                    {
                             /** For title return string without new line character */
                             loc.lines(1);
                             std::string str(yytext);
-                            stringutils::trim_newline(str);
+                            str = stringutils::trim_newline(str);
                             BEGIN(INITIAL);
                             return NmodlParser::make_LINE_PART(str, loc);
                         }

diff --git a/src/lexer/nmodl_utils.cpp b/src/lexer/nmodl_utils.cpp
@@ -93,7 +93,7 @@ SymbolType name_symbol(const std::string& text, PositionType& pos, TokenType typ
 SymbolType prime_symbol(std::string text, PositionType& pos) {
     ModToken token(text, Token::PRIME, pos);
     auto order = std::count(text.begin(), text.end(), '\'');
-    stringutils::remove_character(text, '\'');
+    text = stringutils::remove_character(text, '\'');
 
     auto prime_name = new ast::String(text);
     assert(order <= std::numeric_limits<int>::max());

diff --git a/src/parser/diffeq_driver.cpp b/src/parser/diffeq_driver.cpp
@@ -27,7 +27,7 @@ void DiffeqDriver::parse_equation(const std::string& equation,
     auto const wide_order = std::count(state.begin(), state.end(), '\'');
     assert(wide_order >= 0 && wide_order <= std::numeric_limits<int>::max());
     order = static_cast<int>(wide_order);
-    stringutils::remove_character(state, '\'');
+    state = stringutils::remove_character(state, '\'');
 
     /// error if no prime in equation or not an assignment statement
     if (order == 0 || state.empty()) {

diff --git a/src/symtab/symbol_properties.hpp b/src/symtab/symbol_properties.hpp
@@ -272,13 +272,16 @@ std::vector<std::string> to_string_vector(const syminfo::Status& obj);
 
 template <typename T>
 std::string to_string(const T& obj) {
-    auto elements = to_string_vector(obj);
     std::string text;
-    for (const auto& element: elements) {
-        text += element + " ";
+    bool is_first{true};
+    for (const auto& element: to_string_vector(obj)) {
+        if (is_first) {
+            text += element;
+            is_first = false;
+        } else {
+            text += " " + element;
+        }
     }
-    // remove extra whitespace at the end
-    stringutils::trim(text);
     return text;
 }
 

diff --git a/src/utils/string_utils.hpp b/src/utils/string_utils.hpp
@@ -27,62 +27,95 @@ namespace nmodl {
 namespace stringutils {
 
 /**
- * @addtogroup utils
- * @{
+ * \addtogroup utils
+ * \{
  */
 
 /// text alignment when printing in the tabular form
 enum class text_alignment { left, right, center };
 
-/// Trim from start
-static inline std::string& ltrim(std::string& s) {
-    s.erase(s.begin(), std::find_if(s.begin(), s.end(), [](int c) { return !std::isspace(c); }));
-    return s;
+/**
+ * \param text the string to manipulate
+ * \return a copy of the given string with leading ASCII space characters removed
+ */
+[[nodiscard]] static inline std::string ltrim(std::string text) {
+    text.erase(text.begin(),
+               std::find_if(text.begin(), text.end(), [](int c) { return !std::isspace(c); }));
+    return text;
 }
 
-/// Trim from end
-static inline std::string& rtrim(std::string& s) {
-    s.erase(std::find_if(s.rbegin(), s.rend(), [](int c) { return !std::isspace(c); }).base(),
-            s.end());
-    return s;
+/**
+ * \param text the string to manipulate
+ * \return a copy of the given string with trailing characters removed.
+ */
+[[nodiscard]] static inline std::string rtrim(std::string text) {
+    text.erase(
+        std::find_if(text.rbegin(), text.rend(), [](int c) { return !std::isspace(c); }).base(),
+        text.end());
+    return text;
 }
 
-/// Trim from both ends
-static inline std::string& trim(std::string& s) {
-    return ltrim(rtrim(s));
+/**
+ *
+ * \param text the string to manipulate
+ * \return a copy of the given string with both leading and trailing ASCII space characters removed
+ */
+[[nodiscard]] static inline std::string trim(std::string text) {
+    return ltrim(rtrim(std::move(text)));
 }
 
-static inline void remove_character(std::string& str, const char c) {
-    str.erase(std::remove(str.begin(), str.end(), c), str.end());
+/**
+ * Remove all occurrences of a given character in a text
+ * \param text the string to manipulate
+ * \param c the character to remove
+ * @return a copy the modified text
+ */
+[[nodiscard]] static inline std::string remove_character(std::string text, const char c) {
+    text.erase(std::remove(text.begin(), text.end(), c), text.end());
+    return text;
 }
 
-/// Remove leading newline for the string read by grammar
-static inline std::string& trim_newline(std::string& s) {
-    remove_character(s, '\n');
-    return s;
+/**
+ *
+ * \param text the string to manipulate
+ * \return a copy of the given string with all occurrences of the ASCII newline character removed
+ */
+[[nodiscard]] static inline std::string trim_newline(std::string text) {
+    return remove_character(std::move(text), '\n');
 }
 
-/// for printing json, we have to escape double quotes
-static inline std::string escape_quotes(const std::string& before) {
-    std::string after;
+/**
+ * Escape double-quote in a text, useful for JSON pretty printer.
+ * \param text the string to manipulate
+ * \return a copy of the given string with every " and \ characters prefixed with an extra \
+ */
+[[nodiscard]] static inline std::string escape_quotes(const std::string& text) {
+    std::ostringstream oss;
+    std::string result;
 
-    for (auto c: before) {
+    for (auto c: text) {
         switch (c) {
         case '"':
         case '\\':
-            after += '\\';
+            result += '\\';
             /// don't break here as we want to append actual character
 
         default:
-            after += c;
+            result += c;
         }
     }
 
-    return after;
+    return result;
 }
 
-/// Spilt string with given delimiter and returns vector
-static inline std::vector<std::string> split_string(const std::string& text, char delimiter) {
+/**
+ * Split a text in a list of words, using a given delimiter character
+ * \param text the string to manipulate
+ * \param delimiter the delimiter character
+ * \return a container holding copies of the substrings
+ */
+[[nodiscard]] static inline std::vector<std::string> split_string(const std::string& text,
+                                                                  char delimiter) {
     std::vector<std::string> elements;
     std::stringstream ss(text);
     std::string item;
@@ -94,13 +127,22 @@ static inline std::vector<std::string> split_string(const std::string& text, cha
     return elements;
 }
 
-/// Left/Right/Center-aligns string within a field of width "width"
-static inline std::string align_text(std::string text, int width, text_alignment type) {
+///
+/**
+ * Aligns a text within a field of width \a width
+ * \param text the string to manipulate
+ * \param width the width of the field
+ * \param type the kind of alignment Left, Right, or Center
+ * \return a copy of the string aligned
+ */
+[[nodiscard]] static inline std::string align_text(const std::string& text,
+                                                   int width,
+                                                   text_alignment type) {
     /// left and right spacing
     std::string left, right;
 
     /// count excess room to pad
-    int padding = width - text.size();
+    const int padding = width - static_cast<int>(text.size());
 
     if (padding > 0) {
         if (type == text_alignment::left) {
@@ -111,7 +153,7 @@ static inline std::string align_text(std::string text, int width, text_alignment
             left = std::string(padding / 2, ' ');
             right = std::string(padding / 2, ' ');
             /// if odd #, add one more space
-            if (padding > 0 && padding % 2 != 0) {
+            if (padding % 2 != 0) {
                 right += " ";
             }
         }
@@ -120,7 +162,11 @@ static inline std::string align_text(std::string text, int width, text_alignment
 }
 
 /// To lower case
-static inline std::string tolower(std::string text) {
+/**
+ * \param text the string to manipulate
+ * \return a copy of string converted to lowercase
+ */
+[[nodiscard]] static inline std::string tolower(std::string text) {
     std::transform(text.begin(), text.end(), text.begin(), ::tolower);
     return text;
 }
@@ -135,7 +181,7 @@ static inline std::string tolower(std::string text) {
  */
 std::string to_string(double value, const std::string& format_spec = "{:.16g}");
 
-/** @} */  // end of utils
+/** \} */  // end of utils
 
 }  // namespace stringutils
 }  // namespace nmodl
diff --git a/src/visitors/rename_visitor.hpp b/src/visitors/rename_visitor.hpp
@@ -70,19 +70,19 @@ class RenameVisitor: public ConstAstVisitor {
   public:
     RenameVisitor() = default;
 
-    RenameVisitor(std::string old_name, std::string new_name)
-        : var_name_regex(std::move(old_name))
+    RenameVisitor(const std::string& old_name, std::string new_name)
+        : var_name_regex(old_name)
         , new_var_name(std::move(new_name)) {}
 
     RenameVisitor(std::shared_ptr<ast::Program> ast,
-                  std::string old_name,
+                  const std::string& old_name,
                   std::string new_var_name_or_prefix,
                   bool add_prefix,
                   bool add_random_suffix)
         : ast(std::move(ast))
-        , var_name_regex(std::move(old_name))
-        , add_prefix(std::move(add_prefix))
-        , add_random_suffix(std::move(add_random_suffix)) {
+        , var_name_regex(old_name)
+        , add_prefix(add_prefix)
+        , add_random_suffix(add_random_suffix) {
         if (add_prefix) {
             new_var_name_prefix = std::move(new_var_name_or_prefix);
         } else {
@@ -94,8 +94,8 @@ class RenameVisitor: public ConstAstVisitor {
     /// to the renamed_variables map
     std::string new_name_generator(const std::string& old_name);
 
-    void set(std::string old_name, std::string new_name) {
-        var_name_regex = std::move(old_name);
+    void set(const std::string& old_name, std::string new_name) {
+        var_name_regex = old_name;
         new_var_name = std::move(new_name);
     }
 

diff --git a/test/unit/utils/test_utils.cpp b/test/unit/utils/test_utils.cpp
@@ -20,16 +20,15 @@ namespace test_utils {
 
 int count_leading_spaces(std::string text) {
     auto const length = text.size();
-    nmodl::stringutils::ltrim(text);
+    text = nmodl::stringutils::ltrim(text);
     auto const num_whitespaces = length - text.size();
     assert(num_whitespaces <= std::numeric_limits<int>::max());
     return static_cast<int>(num_whitespaces);
 }
 
 /// check if string has only whitespaces
-bool is_empty(std::string text) {
-    nmodl::stringutils::trim(text);
-    return text.empty();
+bool is_empty(const std::string& text) {
+    return nmodl::stringutils::trim(text).empty();
 }
 
 /** Reindent nmodl text for text-to-text comparison