On 06/27/2013 08:25 PM, Wenchao Xia wrote: >>> + readline_add_completion(mon->rs, file); >>> } >>> } >>> closedir(ffs); >> >> Oops, I started reviewing that before noticing you posted a v3. At any >> rate, my review of v2 still stands on this patch. Meanwhile, I just >> barely noticed that this pre-existing code might cause a -Wshadow >> warning in the future if <strings.h> is ever included. I'm not sure >> what qemu's policy is on being clean against -Wshadow warnings, but I >> personally tend to avoid local variables whose name might conflict with >> global functions. >> > Thanks for reviewing. I'll fix what you mentioned in v2. > > For the -Wshadow warning, I guess you mean "file" right?
No, I meant ffs. ffs() is a global function, in scope if <strings.h> is included; so using it as a local variable name will trigger -Wshadow warnings from (at least some versions of) gcc. > It would > be a bit hard to check this warning for new patch, since this warning > now exist in many code. In my opinion, it would be better to have a > separate series to clean up this warnings first, then check new patches > for this issue. Agreed that the issue (of using ffs as a local variable name) is pre-existing, and therefore cleanups for -Wshadow are not related to this series and not something you need to necessarily worry about. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
