| Message ID | 20190324122121.6430-2-Hi-Angel@yandex.ru |
|---|---|
| State | New |
| Headers | show |
| Series | Minor LGTM fixes | expand |
* 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.
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.
* 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.
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.
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; }
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(-)