On Mon, Mar 19, 2012 at 09:43:26AM -0600, Eric Blake wrote:
> On 03/19/2012 06:43 AM, Daniel P. Berrange wrote:
> >>> That is a historical remant. Do feel free to send another followup patch
> >>> to
> >>> cleanup all the cases of 'return(NULL)' as well.
> >>>
> >>> Daniel
> >>
> >> Did you mean in that file or globally? Because I just tried the first
> >> thing that came to my mind and look at the output:
> >>
> >> libvirt $ find . -name '*.[ch]' -type f -exec grep -nH -e \
> >> 'return(.*);' {} + | wc -l
> >> 852
>
> Yes, it is a rather big cleanup, which is all the more reason why we'd
> want to ensure that 'make syntax-check' prevents it from recurring.
>
> >>
> >> Not that I wouldn't do it, it just seem as a pretty big change. On the
> >> other hand, I don't see the point in changing that in only one file.
> >
> > IMHO, we should do this to make our code consistent, and ideally add a
> > syntax-check rule to prevent them coming back. Anyone disagree ?
>
> I'm in favor of such a cleanup. There are still a few places where
> return needs parenthesis; for example, I'm fond of the style:
>
> return (big_long_conditional ?
> option_1 :
> option_2);
Yes, that usage scenario is fine.
> since the open '(' lets the rest of the code indent nicely when using
> default emacs indentation. But it's still pretty easy to recognize the
> difference between complex returns and the real offenders. I think the
> number of false positives and false negatives is pretty near zero if you
> boil it down to detecting uses where there are no spaces between the '('
> and ')'. Thus, for cfg.mk, I suggest a pattern something like:
>
> sc_prohibit_return_as_function:
> @prohibit='\<return *([^ ]*)' \
> halt='avoid extra () with return statements' \
> $(_sc_search_regexp)
Looks good.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
