diff mbox

remove nested functions from elf/dl-lookup.c

Message ID CAGQ9bdwXd0AVUKRwtosJw+d5QujmqNspeVHG+2N9O+o1_F-KqQ@mail.gmail.com
State Committed
Headers

Commit Message

Kostya Serebryany Oct. 6, 2014, 7:49 p.m. UTC 
  Hi,

Please review the patch that removes a nested function from elf/dl-lookup.c.
This just moves a nested function outside -- no change in parameters.

No regressions in 'make check' on x86_64-linux-gnu (Ubuntu 14.04)

2014-10-06  Kostya Serebryany  <konstantin.s.serebryany@gmail.com>

        * elf/dl-lookup.c
        (enter): New function broken out do_lookup_unique.
        (do_lookup_unique): Remove that nested function.

Comments

Andreas Schwab Oct. 6, 2014, 9:03 p.m. UTC | #1 
Konstantin Serebryany <konstantin.s.serebryany@gmail.com> writes:

>         * elf/dl-lookup.c
>         (enter): New function broken out do_lookup_unique.
>         (do_lookup_unique): Remove that nested function.

Ok.  That can be written like this:

	* elf/dl-lookup.c (enter): Move to file scope.


Andreas.
Roland McGrath Oct. 8, 2014, 10:18 p.m. UTC | #2 
> +/* We have to determine whether we already found a
> +   symbol with this name before.  If not then we have to
> +   add it to the search table.  If we already found a
> +   definition we have to use it.  */
[...]
> -  /* We have to determine whether we already found a
> -     symbol with this name before.  If not then we have to
> -     add it to the search table.  If we already found a
> -     definition we have to use it.  */

When you move a comment to a different indentation level, it's worth
hitting M-q just to see if it comes out prettier in fewer lines.

However, in this case the comment should not have been moved.
It's describing the logic of do_lookup_unique, not of enter.
The name "enter" is a bit too generic for a file-scope function.

I fixed these things up and committed for you.


Thanks,
Roland
Kostya Serebryany Oct. 8, 2014, 10:26 p.m. UTC | #3 
Thanks!

On Wed, Oct 8, 2014 at 3:18 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> +/* We have to determine whether we already found a
>> +   symbol with this name before.  If not then we have to
>> +   add it to the search table.  If we already found a
>> +   definition we have to use it.  */
> [...]
>> -  /* We have to determine whether we already found a
>> -     symbol with this name before.  If not then we have to
>> -     add it to the search table.  If we already found a
>> -     definition we have to use it.  */
>
> When you move a comment to a different indentation level, it's worth
> hitting M-q just to see if it comes out prettier in fewer lines.
>
> However, in this case the comment should not have been moved.
> It's describing the logic of do_lookup_unique, not of enter.
> The name "enter" is a bit too generic for a file-scope function.
>
> I fixed these things up and committed for you.
>
>
> Thanks,
> Roland
diff mbox

Patch

diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index 7c32830..5d3c384 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -182,6 +182,30 @@  check_match (const char *const undef_name,
   return sym;
 }
 
+/* We have to determine whether we already found a
+   symbol with this name before.  If not then we have to
+   add it to the search table.  If we already found a
+   definition we have to use it.  */
+static void
+enter (struct unique_sym *table, size_t size,
+       unsigned int hash, const char *name,
+       const ElfW(Sym) *sym, const struct link_map *map)
+{
+  size_t idx = hash % size;
+  size_t hash2 = 1 + hash % (size - 2);
+  while (table[idx].name != NULL)
+    {
+      idx += hash2;
+      if (idx >= size)
+	idx -= size;
+    }
+
+  table[idx].hashval = hash;
+  table[idx].name = name;
+  table[idx].sym = sym;
+  table[idx].map = map;
+}
+
 /* Utility function for do_lookup_x. Lookup an STB_GNU_UNIQUE symbol
    in the unique symbol table, creating a new entry if necessary.
    Return the matching symbol in RESULT.  */
@@ -191,29 +215,6 @@  do_lookup_unique (const char *undef_name, uint_fast32_t new_hash,
 		  int type_class, const ElfW(Sym) *sym, const char *strtab,
 		  const ElfW(Sym) *ref, const struct link_map *undef_map)
 {
-  /* We have to determine whether we already found a
-     symbol with this name before.  If not then we have to
-     add it to the search table.  If we already found a
-     definition we have to use it.  */
-  void enter (struct unique_sym *table, size_t size,
-	      unsigned int hash, const char *name,
-	      const ElfW(Sym) *sym, const struct link_map *map)
-  {
-    size_t idx = hash % size;
-    size_t hash2 = 1 + hash % (size - 2);
-    while (table[idx].name != NULL)
-      {
-	idx += hash2;
-	if (idx >= size)
-	  idx -= size;
-      }
-
-    table[idx].hashval = hash;
-    table[idx].name = name;
-    table[idx].sym = sym;
-    table[idx].map = map;
-  }
-
   struct unique_sym_table *tab
     = &GL(dl_ns)[map->l_ns]._ns_unique_sym_table;