Re: [PATCH 4/4] Use std::string::substr instead of appending single chars

Re: [PATCH 4/4] Use std::string::substr instead of appending single chars

[Dodji Seketeli]

Jonathan Wakely <jwakely@redhat.com> a écrit:

> Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
> ---
>  src/abg-ini.cc | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)

Added a ChangeLog part to the commit message and applied to master.

Thanks!

-- 
		Dodji

[PATCH 4/4] Use std::string::substr instead of appending single chars

[Jonathan Wakely]

Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
---
 src/abg-ini.cc | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/abg-ini.cc b/src/abg-ini.cc
index 7558c2f..8320e1f 100644
--- a/src/abg-ini.cc
+++ b/src/abg-ini.cc
@@ -176,8 +176,6 @@ char_is_white_space(int b)
 static string
 trim_white_space(const string& str)
 {
-  string result;
-
   if (str.empty())
     return str;
 
@@ -191,10 +189,7 @@ trim_white_space(const string& str)
     if (!char_is_white_space(str[e]))
       break;
 
-  for (unsigned i = s; i <= e; ++i)
-    result += str[i];
-
-  return result;
+  return str.substr(s, e - s + 1);
 }
 
 // <property stuff>
-- 
2.13.6

Re: [PATCH 1/4] Remove assertion with side-effects

[Dodji Seketeli]

Hello Jonathan,

Jonathan Wakely <jwakely@redhat.com> a écrit:

> Calling assert(split_string(...)) won't do anything when NDEBUG is
> defined, but the split_string call can be avoided anyway.
>
> Since only the first result from the split string is needed, and
> remove_trailing_white_spaces will trim white space anyway, the overhead
> of parsing it into a vector can be avoided by using std::string::substr
> directly. Additionally, calling std::string::find with a single char is
> more efficient than with a string.

Applied to master.  I just added a ChangeLog part to it.

Thanks!

-- 
		Dodji

[PATCH 1/4] Remove assertion with side-effects

[Jonathan Wakely]

Calling assert(split_string(...)) won't do anything when NDEBUG is
defined, but the split_string call can be avoided anyway.

Since only the first result from the split string is needed, and
remove_trailing_white_spaces will trim white space anyway, the overhead
of parsing it into a vector can be avoided by using std::string::substr
directly. Additionally, calling std::string::find with a single char is
more efficient than with a string.

Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
---
 src/abg-tools-utils.cc | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index 1f25ef3..60bb773 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -864,24 +864,10 @@ get_dsos_provided_by_rpm(const string& rpm_path, set<string>& provided_dsos)
        line != query_output.end();
        ++line)
     {
-      vector<string> splitted;
-      string dso;
-      if (!line->empty())
-	{
-	  if (line->find("(") != string::npos)
-	    {
-	      assert(split_string(*line, "(", splitted));
-	      dso = splitted.front();
-	    }
-	  else
-	    dso = *line;
-	}
-
+      string dso = line->substr(0, line->find('('));
+      dso = remove_trailing_white_spaces(dso);
       if (!dso.empty())
-	{
-	  dso = remove_trailing_white_spaces(dso);
-	  provided_dsos.insert(dso);
-	}
+	provided_dsos.insert(dso);
     }
   return true;
 }
-- 
2.13.6

Re: [PATCH 3/4] Rename misleading remove_trailing_white_spaces functions

[Dodji Seketeli]

Jonathan Wakely <jwakely@redhat.com> a écrit:

> Trailing implies only whitespace at the end is removed, but these
> functions also remove leading whitespace.
>
> Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
> ---
>  include/abg-tools-utils.h |  2 +-
>  src/abg-ini.cc            | 13 +++++--------
>  src/abg-tools-utils.cc    |  7 +++----
>  3 files changed, 9 insertions(+), 13 deletions(-)

Added a ChangeLog part to the commit message and applied to master.

Thanks!

-- 
		Dodji

Re: [PATCH 2/4] Remove unused local set<string> variables

[Dodji Seketeli]

Jonathan Wakely <jwakely@redhat.com> a écrit:

> Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
> ---
>  src/abg-tools-utils.cc | 1 -
>  tools/abipkgdiff.cc    | 5 +----
>  2 files changed, 1 insertion(+), 5 deletions(-)

Added a ChangeLog part to the commit message and applied to master.

Thanks!

-- 
		Dodji

[PATCH 3/4] Rename misleading remove_trailing_white_spaces functions

[Jonathan Wakely]

Trailing implies only whitespace at the end is removed, but these
functions also remove leading whitespace.

Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
---
 include/abg-tools-utils.h |  2 +-
 src/abg-ini.cc            | 13 +++++--------
 src/abg-tools-utils.cc    |  7 +++----
 3 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/include/abg-tools-utils.h b/include/abg-tools-utils.h
index defc7ba..2e59256 100644
--- a/include/abg-tools-utils.h
+++ b/include/abg-tools-utils.h
@@ -71,7 +71,7 @@ string get_library_version_string();
 bool execute_command_and_get_output(const string&, vector<string>&);
 bool get_dsos_provided_by_rpm(const string& rpm_path,
 			      set<string>& provided_dsos);
-string remove_trailing_white_spaces(const string&);
+string trim_white_space(const string&);
 suppr::type_suppression_sptr
 gen_suppr_spec_from_headers(const string& hdrs_root_dir);
 
diff --git a/src/abg-ini.cc b/src/abg-ini.cc
index c33e2ae..7558c2f 100644
--- a/src/abg-ini.cc
+++ b/src/abg-ini.cc
@@ -168,15 +168,13 @@ static bool
 char_is_white_space(int b)
 {return b == ' ' || b == '\t' || b == '\n';}
 
-/// Remove the trailing spaces at the begining and at the end of a
-/// given string.
+/// Remove the spaces at the begining and at the end of a given string.
 ///
-/// @param str the string to remove trailing white spaces from.
+/// @param str the string to remove leading and trailing white spaces from.
 ///
-/// @return the string resulting from the removal of trailing white
-/// space from @p str.
+/// @return the string resulting from the removal of white space from @p str.
 static string
-remove_trailing_white_spaces(const string& str)
+trim_white_space(const string& str)
 {
   string result;
 
@@ -1401,8 +1399,7 @@ public:
 	assert(read_next_char(c));
 	v += c;
       }
-    string result = remove_trailing_white_spaces(v);
-    return result;
+    return trim_white_space(v);
   }
 
   /// Read a string property value.
diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index 38c4940..c560fb0 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -864,7 +864,7 @@ get_dsos_provided_by_rpm(const string& rpm_path, set<string>& provided_dsos)
        ++line)
     {
       string dso = line->substr(0, line->find('('));
-      dso = remove_trailing_white_spaces(dso);
+      dso = trim_white_space(dso);
       if (!dso.empty())
 	provided_dsos.insert(dso);
     }
@@ -875,10 +875,9 @@ get_dsos_provided_by_rpm(const string& rpm_path, set<string>& provided_dsos)
 ///
 /// @param str the input string to consider.
 ///
-/// @return the @p str string from which trailing white spaces have
-/// been removed.
+/// @return the @p str string with leading and trailing white spaces removed.
 string
-remove_trailing_white_spaces(const string& str)
+trim_white_space(const string& str)
 {
   if (str.empty())
     return "";
-- 
2.13.6

[PATCH 2/4] Remove unused local set<string> variables

[Jonathan Wakely]

Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
---
 src/abg-tools-utils.cc | 1 -
 tools/abipkgdiff.cc    | 5 +----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index 60bb773..38c4940 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -859,7 +859,6 @@ get_dsos_provided_by_rpm(const string& rpm_path, set<string>& provided_dsos)
 				      query_output))
     return false;
 
-  set<string> dsos;
   for (vector<string>::const_iterator line = query_output.begin();
        line != query_output.end();
        ++line)
diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index 9f0ac6f..2bb81fe 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -1541,10 +1541,7 @@ maybe_create_public_dso_sonames_set(package& pkg, const options &opts)
     return false;
 
   if (pkg.type() == abigail::tools_utils::FILE_TYPE_RPM)
-    {
-      set<string> public_dsos;
-      return get_dsos_provided_by_rpm(pkg.path(), pkg.public_dso_sonames());
-    }
+    return get_dsos_provided_by_rpm(pkg.path(), pkg.public_dso_sonames());
 
   // We don't support this yet for non-RPM packages.
   return false;
-- 
2.13.6