Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.03.2020 16:38, Andrew Cooper wrote:
> On 23/03/2020 08:38, Jan Beulich wrote:
>> On 21.03.2020 23:19, Julien Grall wrote:
>>> On Fri, 20 Mar 2020 at 21:26, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
>>> wrote:
>>>> --- a/xen/include/xen/xmalloc.h
>>>> +++ b/xen/include/xen/xmalloc.h
>>>> @@ -51,6 +51,17 @@
>>>>  #define xmalloc_bytes(_bytes) _xmalloc(_bytes, SMP_CACHE_BYTES)
>>>>  #define xzalloc_bytes(_bytes) _xzalloc(_bytes, SMP_CACHE_BYTES)
>>>>
>>>> +/* Allocate untyped storage and copying an existing instance. */
>>>> +#define xmemdup_bytes(_src, _nr)                \
>>>> +    ({                                          \
>>>> +        unsigned long nr_ = (_nr);              \
>>>> +        void *dst_ = xmalloc_bytes(nr_);        \
>>> The nr_ vs _nr is really confusing to read. Could you re-implement the
>>> function as a static inline?
>> And even if that wouldn't work out - what's the point of having
>> macro argument names with leading underscores?
> 
> Consistency with all the other code in this file.

I value consistency quite high, but then please consistent with
something that properly follows other rules.

>>  This isn't any
>> better standard-wise (afaict) than other uses of leading
>> underscores for identifiers which aren't CU-scope.
> 
> It is a parameter describing textural replacement within the body. 
> There is 0 interaction with external namespacing standards.

Please forgive me saying so, but a typical reply I might get back
from you would be: And?

I'm not going to insist nor nak the patch, but I don't welcome
widening existing issues we have.

Jan