remove one nested function from nptl/allocatestack.c

Message ID 20140605090455.GB9145@spoyarek.pnq.redhat.com
State Superseded
Headers

Commit Message

Siddhesh Poyarekar June 5, 2014, 9:04 a.m. UTC
  On Wed, Jun 04, 2014 at 10:16:23AM -0700, Roland McGrath wrote:
> That is fine, though it wouldn't hurt to improve the commentary and use
> bool while you're there.

I wonder if it would be better to just inline this function:
  

Comments

Kostya Serebryany June 5, 2014, 9:10 a.m. UTC | #1
On Thu, Jun 5, 2014 at 1:04 PM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> On Wed, Jun 04, 2014 at 10:16:23AM -0700, Roland McGrath wrote:
>> That is fine, though it wouldn't hurt to improve the commentary and use
>> bool while you're there.
>
> I wonder if it would be better to just inline this function:

This seems to be equally ok.

>
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 1e22f7d..b779529 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -830,26 +830,23 @@ __reclaim_stacks (void)
>
>        if (add_p)
>         {
> -         /* We always add at the beginning of the list.  So in this
> -            case we only need to check the beginning of these lists.  */
> -         int check_list (list_t *l)
> -         {
> -           if (l->next->prev != l)
> -             {
> -               assert (l->next->prev == elem);
> -
> -               elem->next = l->next;
> -               elem->prev = l;
> -               l->next = elem;
> -
> -               return 1;
> -             }
> -
> -           return 0;
> -         }
> -
> -         if (check_list (&stack_used) == 0)
> -           (void) check_list (&stack_cache);
> +         /* We always add at the beginning of the list.  So in this case we
> +            only need to check the beginning of these lists to see if the
> +            pointers at the head of the list are inconsistent.  */
> +         list_t *l = NULL;
> +
> +         if (stack_used.next->prev != &stack_used)
> +           l = &stack_used;
> +         else if (stack_cache.next->prev != &stack_cache)
> +           l = &stack_cache;
> +
> +         if (l)
> +           {
> +             assert (l->next->prev == elem);
> +             elem->next = l->next;
> +             elem->prev = l;
> +             l->next = elem;
> +           }
>         }
>        else
>         {
  
Andreas Schwab June 5, 2014, 9:10 a.m. UTC | #2
Siddhesh Poyarekar <siddhesh@redhat.com> writes:

> I wonder if it would be better to just inline this function:

This is much better.

Andreas.
  
Roland McGrath June 5, 2014, 6:31 p.m. UTC | #3
> +	  /* We always add at the beginning of the list.  So in this case we
> +	     only need to check the beginning of these lists to see if the
> +	     pointers at the head of the list are inconsistent.  */
> +	  list_t *l = NULL;
> +
> +	  if (stack_used.next->prev != &stack_used)
> +	    l = &stack_used;
> +	  else if (stack_cache.next->prev != &stack_cache)
> +	    l = &stack_cache;
> +
> +	  if (l)

No implicit Boolean coercion.

Otherwise this is fine.
  

Patch

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 1e22f7d..b779529 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -830,26 +830,23 @@  __reclaim_stacks (void)
 
       if (add_p)
 	{
-	  /* We always add at the beginning of the list.  So in this
-	     case we only need to check the beginning of these lists.  */
-	  int check_list (list_t *l)
-	  {
-	    if (l->next->prev != l)
-	      {
-		assert (l->next->prev == elem);
-
-		elem->next = l->next;
-		elem->prev = l;
-		l->next = elem;
-
-		return 1;
-	      }
-
-	    return 0;
-	  }
-
-	  if (check_list (&stack_used) == 0)
-	    (void) check_list (&stack_cache);
+	  /* We always add at the beginning of the list.  So in this case we
+	     only need to check the beginning of these lists to see if the
+	     pointers at the head of the list are inconsistent.  */
+	  list_t *l = NULL;
+
+	  if (stack_used.next->prev != &stack_used)
+	    l = &stack_used;
+	  else if (stack_cache.next->prev != &stack_cache)
+	    l = &stack_cache;
+
+	  if (l)
+	    {
+	      assert (l->next->prev == elem);
+	      elem->next = l->next;
+	      elem->prev = l;
+	      l->next = elem;
+	    }
 	}
       else
 	{