[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Fixed NULL pointer check



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]