Re: [Qemu-devel] [PATCH 01/23] memory: introduce memory_region_find()

[Anthony Liguori <anthony@codemonkey.ws>]

On 12/19/2011 09:17 AM, Avi Kivity wrote:
> On 12/19/2011 05:10 PM, Anthony Liguori wrote:
>> On 12/19/2011 09:04 AM, Avi Kivity wrote:
>>> On 12/19/2011 04:50 PM, Anthony Liguori wrote:
>>>>> +static int cmp_flatrange_addr(const void *_addr, const void *_fr)
>>>>> +{
>>>>> +    const AddrRange *addr = _addr;
>>>>> +    const FlatRange *fr = _fr;
>>>>
>>>>
>>>> Please don't prefix with an underscore.
>>>
>>> Why not?  It's legal according to the standards, if that's your concern
>>> (only _[_A-Z]+ are reserved).
>>
>> http://www.gnu.org/s/hello/manual/libc/Reserved-Names.html
>>
>> "In addition to the names documented in this manual, reserved names
>> include all external identifiers (global functions and variables) that
>> begin with an underscore (‘_’)"
>>
>> Now that might just mean that you're shadowing a global name, so maybe
>> that's okay, but I think it's easier to just enforce a consistent rule
>> of, "don't start identifiers with an underscore".
>
> I rather like the pattern
>
>     void callback(void *_foo)
>     {
>           Foo *foo = _foo;
>
>           ...
>     }
>
> If the consensus is that we don't want it, I'll change it, but I do
> think it's useful.

I dislike enforcing coding style but it's a necessary evil. I greater prefer 
simple rules to subtle ones as it creates less confusion so I'd prefer you 
change this.

>
>>>>> +MemoryRegionSection memory_region_find(MemoryRegion *address_space,
>>>>> +                                       target_phys_addr_t addr,
>>>>> uint64_t size);
>>>>
>>>>
>>>> Returning structs by value is a bit unexpected.
>>>
>>> It's just prejudice, here's the call sequence:
>>
>> It's not about whether it's fast or slow.  It's just unexpected.
>
> It's plain C, I don't see why it's so unexpected.
>
>>
>> Instead of returning a NULL pointer on error, you set .mr to NULL if
>> an error occurs (which isn't documented by the comment btw).
>> Returning a pointer, or taking a pointer and returning a 0/-1 return
>> value makes for a more consistent semantic with the rest of the code
>> base.
>>
>
> How can I return a pointer?  If I allocate it, someone has to free it.
> If I pass it as a parameter, we end up with a silly looking calling
> convention.  If I return an error code, the caller has to set up an
> additional variable.
>
> The natural check is section.size which is always meaningful -
> memory_region_find() returns a subrange of the input, which may be
> zero.  You're right that I should document it (and I should use it
> consistently).

I'm not going to make a fuss over something like this so if you really prefer 
this style, I'm not going to object strongly.

But it should be at least be documented and used consistently.

Regards,

Anthony Liguori