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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
