[RFC] Cleanup breakpoint_re_set_one

[RFC] Cleanup breakpoint_re_set_one

[Thiago Jung Bauermann]

Hi,

I was studying breakpoint_re_set_one and had to scratch my head for a
while before I could understand the error handling logic in the
breakpoints case. There's an unusually high incidence of double
negatives and booleans which when true mean that something is _not_
going on...

I considered changing b->condition_not_parsed to b->condition_parsed,
but that would be a very big and error-prone change. IMHO this
rearrangement of the expression is a good first step.

Changing decode_line_1's not_found to found would probably be easier,
but since I don't have a pressing need for that I didn't attempt to do
so here. I can provide a separate patch if you think it's worthwhile.

Tested with no regressions on ppc-linux and ppc64-linux. WDYT?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2011-03-21  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	* breakpoint.c (breakpoint_re_set_one): Cleanup logic of the
	conditon expressions in the error handling code for breakpoints.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c55e5c0..16fc508 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10545,33 +10545,26 @@ breakpoint_re_set_one (void *bint)
 	    sals = decode_line_1 (&s, 1, (struct symtab *) NULL, 0,
 				  (char ***) NULL, not_found_ptr);
 	}
-      if (e.reason < 0)
+
+      /* For pending breakpoints, it's expected that parsing will
+	 fail until the right shared library is loaded.  User has
+	 already told to create pending breakpoints and don't need
+	 extra messages.  If breakpoint is in bp_shlib_disabled
+	 state, then user already saw the message about that
+	 breakpoint being disabled, and don't want to see more
+	 errors.  */
+      if (e.reason < 0 && (!not_found || (!b->condition_not_parsed
+					   && !(b->loc && b->loc->shlib_disabled)
+					   && b->enable_state != bp_disabled)))
 	{
-	  int not_found_and_ok = 0;
-	  /* For pending breakpoints, it's expected that parsing will
-	     fail until the right shared library is loaded.  User has
-	     already told to create pending breakpoints and don't need
-	     extra messages.  If breakpoint is in bp_shlib_disabled
-	     state, then user already saw the message about that
-	     breakpoint being disabled, and don't want to see more
-	     errors.  */
-	  if (not_found 
-	      && (b->condition_not_parsed 
-		  || (b->loc && b->loc->shlib_disabled)
-		  || b->enable_state == bp_disabled))
-	    not_found_and_ok = 1;
-
-	  if (!not_found_and_ok)
-	    {
-	      /* We surely don't want to warn about the same breakpoint
-		 10 times.  One solution, implemented here, is disable
-		 the breakpoint on error.  Another solution would be to
-		 have separate 'warning emitted' flag.  Since this
-		 happens only when a binary has changed, I don't know
-		 which approach is better.  */
-	      b->enable_state = bp_disabled;
-	      throw_exception (e);
-	    }
+	  /* We surely don't want to warn about the same breakpoint
+	     10 times.  One solution, implemented here, is disable
+	     the breakpoint on error.  Another solution would be to
+	     have separate 'warning emitted' flag.  Since this
+	     happens only when a binary has changed, I don't know
+	     which approach is better.  */
+	  b->enable_state = bp_disabled;
+	  throw_exception (e);
 	}
 
       if (!not_found)

Re: [RFC] Cleanup breakpoint_re_set_one

[Thiago Jung Bauermann]

Hi,

On Mon, 2011-03-21 at 18:14 -0300, Thiago Jung Bauermann wrote:
> Tested with no regressions on ppc-linux and ppc64-linux. WDYT?

This is a refreshed version of the patch, which applies on
today's CVS HEAD.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2011-04-01  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	* breakpoint.c (addr_string_to_sals): Cleanup logic of the
	conditon expressions in the error handling code.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3b1367b..6d7c234 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10979,33 +10979,26 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
 	sals = decode_line_1 (&s, 1, (struct symtab *) NULL, 0,
 			      NULL, &not_found);
     }
-  if (e.reason < 0)
-    {
-      int not_found_and_ok = 0;
-      /* For pending breakpoints, it's expected that parsing will
-	 fail until the right shared library is loaded.  User has
-	 already told to create pending breakpoints and don't need
-	 extra messages.  If breakpoint is in bp_shlib_disabled
-	 state, then user already saw the message about that
-	 breakpoint being disabled, and don't want to see more
-	 errors.  */
-      if (not_found 
-	  && (b->condition_not_parsed 
-	      || (b->loc && b->loc->shlib_disabled)
-	      || b->enable_state == bp_disabled))
-	not_found_and_ok = 1;
-
-      if (!not_found_and_ok)
-	{
-	  /* We surely don't want to warn about the same breakpoint
-	     10 times.  One solution, implemented here, is disable
-	     the breakpoint on error.  Another solution would be to
-	     have separate 'warning emitted' flag.  Since this
-	     happens only when a binary has changed, I don't know
-	     which approach is better.  */
-	  b->enable_state = bp_disabled;
-	  throw_exception (e);
-	}
+
+  /* For pending breakpoints, it's expected that parsing will
+     fail until the right shared library is loaded.  User has
+     already told to create pending breakpoints and don't need
+     extra messages.  If breakpoint is in bp_shlib_disabled
+     state, then user already saw the message about that
+     breakpoint being disabled, and don't want to see more
+     errors.  */
+  if (e.reason < 0 && (!not_found || (!b->condition_not_parsed
+				      && !(b->loc && b->loc->shlib_disabled)
+				      && b->enable_state != bp_disabled)))
+    {
+      /* We surely don't want to warn about the same breakpoint
+	 10 times.  One solution, implemented here, is disable
+	 the breakpoint on error.  Another solution would be to
+	 have separate 'warning emitted' flag.  Since this
+	 happens only when a binary has changed, I don't know
+	 which approach is better.  */
+      b->enable_state = bp_disabled;
+      throw_exception (e);
     }
 
   if (!not_found)