diff mbox series

Make decl_names_equal more accurate

Message ID 20200722105723.2547112-1-gprocida@google.com
State New
Headers show
Series Make decl_names_equal more accurate | expand

Commit Message

Giuliano Procida July 22, 2020, 10:57 a.m. UTC
This utility function compares qualified names component-wise with
special handling of components that are anonymous names.

The code is incorrect for some cases when there are different numbers
of components on each side. Also, the control flow in the loop body is
more complex than it needs to be.

This commit simplifies the control flow, fixes the comparison bugs and
adds some extra tests to cover these cases.

	* src/abg-tools-utils.cc (decl_names_equal): Move {l,r}_pos2
	declarations into the loop and make {l,r}_length const. Avoid
	chance of arithmetic on string::npos values. Rework
	logic so there is a single test for "names compare equal" and
	a single test for different numbers of name components.
	* tests/test-tools-utils.cc (main): Add nine more tests.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-tools-utils.cc    | 72 +++++++++++++++------------------------
 tests/test-tools-utils.cc | 10 ++++++
 2 files changed, 38 insertions(+), 44 deletions(-)

Comments

Dodji Seketeli Aug. 4, 2020, 12:57 p.m. UTC | #1
Giuliano Procida <gprocida@google.com> a écrit:

> This utility function compares qualified names component-wise with
> special handling of components that are anonymous names.
>
> The code is incorrect for some cases when there are different numbers
> of components on each side. Also, the control flow in the loop body is
> more complex than it needs to be.

I am not sure the result is radically simpler.  But I agree it fixes the
problem.

Just for the record, I believe this much shorter change also fixes the
issue:

diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index dfbec87..919b50f 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -540,7 +540,7 @@ decl_names_equal(const string& l, const string& r)
 			ANONYMOUS_ENUM_INTERNAL_NAME) == 0))
 	{
 	  if (l_pos2 == l.npos || r_pos2 == r.npos)
-	    return true;
+	    return (l_pos2 == l.npos) ==  (r_pos2 == r.npos);
 
 	  l_pos1 = l_pos2 + 2;
 	  r_pos1 = r_pos2 + 2;

But as I don't have any strong opinion either way, I am going with your
patch.


> This commit simplifies the control flow, fixes the comparison bugs and
> adds some extra tests to cover these cases.
>
> 	* src/abg-tools-utils.cc (decl_names_equal): Move {l,r}_pos2
> 	declarations into the loop and make {l,r}_length const. Avoid
> 	chance of arithmetic on string::npos values. Rework
> 	logic so there is a single test for "names compare equal" and
> 	a single test for different numbers of name components.
> 	* tests/test-tools-utils.cc (main): Add nine more tests.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master, thanks!

Cheers,
diff mbox series

Patch

diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index dfbec879..38e2f175 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -510,61 +510,45 @@  get_anonymous_enum_internal_name_prefix()
 bool
 decl_names_equal(const string& l, const string& r)
 {
-  string::size_type l_pos1 = 0, l_pos2 = 0, r_pos1 = 0, r_pos2 = 0;
-  string::size_type l_length = l.length(), r_length = r.length();
+  string::size_type l_pos1 = 0, r_pos1 = 0;
+  const string::size_type l_length = l.length(), r_length = r.length();
 
   while (l_pos1 < l_length && r_pos1 < r_length)
     {
-      l_pos2 = l.find("::", l_pos1);
-      r_pos2 = r.find("::", r_pos1);
+      string::size_type l_pos2 = l.find("::", l_pos1);
+      string::size_type r_pos2 = r.find("::", r_pos1);
+      if (l_pos2 == string::npos)
+	l_pos2 = l_length;
+      if (r_pos2 == string::npos)
+	r_pos2 = r_length;
 
-      if ((l.compare(l_pos1,
-		     ANONYMOUS_STRUCT_INTERNAL_NAME_LEN,
-		     ANONYMOUS_STRUCT_INTERNAL_NAME) == 0
-	   && r.compare(r_pos1,
+      if (l.compare(l_pos1, l_pos2 - l_pos1, r,
+		    r_pos1, r_pos2 - r_pos1)
+	  && (l.compare(l_pos1,
 			ANONYMOUS_STRUCT_INTERNAL_NAME_LEN,
-			ANONYMOUS_STRUCT_INTERNAL_NAME) == 0)
-	  ||
-	  (l.compare(l_pos1,
-		     ANONYMOUS_UNION_INTERNAL_NAME_LEN,
-		     ANONYMOUS_UNION_INTERNAL_NAME) == 0
-	   && r.compare(r_pos1,
+			ANONYMOUS_STRUCT_INTERNAL_NAME)
+	      || r.compare(r_pos1,
+			   ANONYMOUS_STRUCT_INTERNAL_NAME_LEN,
+			   ANONYMOUS_STRUCT_INTERNAL_NAME))
+	  && (l.compare(l_pos1,
 			ANONYMOUS_UNION_INTERNAL_NAME_LEN,
-			ANONYMOUS_UNION_INTERNAL_NAME) == 0)
-	  ||
-	  (l.compare(l_pos1,
-		     ANONYMOUS_ENUM_INTERNAL_NAME_LEN,
-		     ANONYMOUS_ENUM_INTERNAL_NAME) == 0
-	   && r.compare(r_pos1,
+			ANONYMOUS_UNION_INTERNAL_NAME)
+	      || r.compare(r_pos1,
+			   ANONYMOUS_UNION_INTERNAL_NAME_LEN,
+			   ANONYMOUS_UNION_INTERNAL_NAME))
+	  && (l.compare(l_pos1,
 			ANONYMOUS_ENUM_INTERNAL_NAME_LEN,
-			ANONYMOUS_ENUM_INTERNAL_NAME) == 0))
-	{
-	  if (l_pos2 == l.npos || r_pos2 == r.npos)
-	    return true;
-
-	  l_pos1 = l_pos2 + 2;
-	  r_pos1 = r_pos2 + 2;
-	  continue;
-	}
-
-      if (l_pos2 == l.npos || r_pos2 == r.npos)
-	{
-	  if (l_pos2 != r_pos2)
-	    return false;
-
-	  return !l.compare(l_pos1, l_pos2, r,
-			    r_pos1, r_pos2);
-	}
-
-      if (l.compare(l_pos1, l_pos2 - l_pos1, r,
-		    r_pos1, r_pos2 - r_pos1))
+			ANONYMOUS_ENUM_INTERNAL_NAME)
+	      || r.compare(r_pos1,
+			   ANONYMOUS_ENUM_INTERNAL_NAME_LEN,
+			   ANONYMOUS_ENUM_INTERNAL_NAME)))
 	return false;
 
-      l_pos1 = l_pos2 + 2;
-      r_pos1 = r_pos2 + 2;
+      l_pos1 = l_pos2 == l_length ? l_pos2 : l_pos2 + 2;
+      r_pos1 = r_pos2 == r_length ? r_pos2 : r_pos2 + 2;
     }
 
-  return true;
+  return (l_pos1 == l_length) == (r_pos1 == r_length);
 }
 
 /// If a given file is a symbolic link, get the canonicalized absolute
diff --git a/tests/test-tools-utils.cc b/tests/test-tools-utils.cc
index d29bb8c9..5597873f 100644
--- a/tests/test-tools-utils.cc
+++ b/tests/test-tools-utils.cc
@@ -86,5 +86,15 @@  main(int, char**)
 
   ABG_ASSERT(decl_names_equal("S0::m2", "S0::m12") == false);
 
+  ABG_ASSERT(!decl_names_equal("S0::S1", "S0"));
+  ABG_ASSERT(!decl_names_equal("S0", "S0::S1"));
+  ABG_ASSERT(!decl_names_equal("S1::S0", "S0::S1"));
+  ABG_ASSERT(!decl_names_equal("__anonymous_struct__::S0", "__anonymous_struct__"));
+  ABG_ASSERT(!decl_names_equal("__anonymous_struct__", "__anonymous_struct__::S1"));
+  ABG_ASSERT(!decl_names_equal("__anonymous_struct__::S0", "__anonymous_struct__::S1"));
+  ABG_ASSERT(!decl_names_equal("S0::__anonymous_struct__", "__anonymous_struct__"));
+  ABG_ASSERT(!decl_names_equal("__anonymous_struct__", "S1::__anonymous_struct__"));
+  ABG_ASSERT(!decl_names_equal("S0::__anonymous_struct__", "S1::__anonymous_struct__"));
+
   return 0;
 }