[1/5] support: don't pass to set_search_path a big struct by value
diff mbox series

Message ID 20190324122121.6430-2-Hi-Angel@yandex.ru
State New
Headers show
Series
  • Minor LGTM fixes
Related show

Commit Message

Konstantin Kharlamov March 24, 2019, 12:21 p.m. UTC
Fixes LGTM warning: " This parameter of type resolv_redirect_config is
88 bytes - consider passing a const pointer/reference instead."

Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru>
---
 support/resolv_test.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Florian Weimer March 25, 2019, 8:38 a.m. UTC | #1
* Konstantin Kharlamov:

>  /* Copy the search path from CONFIG.search to the _res object.  */
>  static void
> -set_search_path (struct resolv_redirect_config config)
> +set_search_path (const struct resolv_redirect_config* config)

This static function is called once and therefore inlined, so the
change does not matter.
Konstantin Kharlamov March 27, 2019, 8:50 a.m. UTC | #2
It matters in terms of the code as read by human: α) one would no 
longer wonder why "by-value" is used instead of a "by-pointer", and β) 
the warning by static analyzer ceases to exist.

-----

Thanks for comments! I'll address the other comments (one about testing 
suite + one patch changes ABI, have to be dropped), likely, on the 
weekend, and will resend v2 of the series.

On Пн, Mar 25, 2019 at 09:38, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Konstantin Kharlamov:
> 
>>   /* Copy the search path from CONFIG.search to the _res object.  */
>>   static void
>>  -set_search_path (struct resolv_redirect_config config)
>>  +set_search_path (const struct resolv_redirect_config* config)
> 
> This static function is called once and therefore inlined, so the
> change does not matter.
Florian Weimer March 27, 2019, 8:56 a.m. UTC | #3
* Konstantin Kharlamov:

> It matters in terms of the code as read by human: α) one would no 
> longer wonder why "by-value" is used instead of a "by-pointer",

With by-pointer, the reader has to wonder if the called function
retains a pointer after the call, so it is a wash in this regard.
Konstantin Kharlamov March 27, 2019, 9:12 a.m. UTC | #4
On Ср, Mar 27, 2019 at 09:56, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Konstantin Kharlamov:
> 
>>  It matters in terms of the code as read by human: α) one would no
>>  longer wonder why "by-value" is used instead of a "by-pointer",
> 
> With by-pointer, the reader has to wonder if the called function
> retains a pointer after the call, so it is a wash in this regard.

Sorry, what you mean by "retains"? As in, "stores into a static 
variable for caching purposes"? Well, first of, if a struct contains 
pointers, then it doesn't even matter whether it was passed by-value or 
by-pointer — one could get a data race either way. Second, unless the 
function being explicit about caching a parameter, doing that behind 
the curtains sounds outright wrong as it gonna lead to hard to debug 
bugs.

So, normally I would not assume that an arbitrary function could retain 
its argument.

Patch
diff mbox series

diff --git a/support/resolv_test.c b/support/resolv_test.c
index 903ab2ac05..f400026cd1 100644
--- a/support/resolv_test.c
+++ b/support/resolv_test.c
@@ -1079,7 +1079,7 @@  resolv_test_init (void)
 
 /* Copy the search path from CONFIG.search to the _res object.  */
 static void
-set_search_path (struct resolv_redirect_config config)
+set_search_path (const struct resolv_redirect_config* config)
 {
   memset (_res.defdname, 0, sizeof (_res.defdname));
   memset (_res.dnsrch, 0, sizeof (_res.dnsrch));
@@ -1088,15 +1088,15 @@  set_search_path (struct resolv_redirect_config config)
   char *end = current + sizeof (_res.defdname);
 
   for (unsigned int i = 0;
-       i < sizeof (config.search) / sizeof (config.search[0]); ++i)
+       i < sizeof (config->search) / sizeof (config->search[0]); ++i)
     {
-      if (config.search[i] == NULL)
+      if (config->search[i] == NULL)
         continue;
 
-      size_t length = strlen (config.search[i]) + 1;
+      size_t length = strlen (config->search[i]) + 1;
       size_t remaining = end - current;
       TEST_VERIFY_EXIT (length <= remaining);
-      memcpy (current, config.search[i], length);
+      memcpy (current, config->search[i], length);
       _res.dnsrch[i] = current;
       current += length;
     }
@@ -1203,7 +1203,7 @@  resolv_test_start (struct resolv_redirect_config config)
         }
     }
 
-  set_search_path (config);
+  set_search_path (&config);
 
   return obj;
 }