From maechler at stat.math.ethz.ch  Fri Jul 21 10:21:21 2017
From: maechler at stat.math.ethz.ch (Martin Maechler)
Date: Fri, 21 Jul 2017 10:21:21 +0200
Subject: [Rd] [PATCH] Fix missing break
In-Reply-To: <4241711.WyxJnDo7EK@x2>
References: <60035856.27PnXlZqtm@x2>
 <f4b10bcb-aa8a-75cb-408f-236a24748aed@gmail.com>
 <4241711.WyxJnDo7EK@x2>
Message-ID: <22897.47489.517803.232885@stat.math.ethz.ch>

>>>>> Steve Grubb <sgrubb at redhat.com>
>>>>>     on Thu, 20 Jul 2017 22:20:33 -0400 writes:

    > On Thursday, July 20, 2017 7:41:00 PM EDT Duncan Murdoch wrote:
    >> Thanks for posting this series of patches.  Unfortunately, there's a
    >> good chance they'll get lost in all the traffic on R-devel.  If you
    >> don't hear that they've been fixed in the next couple of weeks, could
    >> you post them to bugs.r-project.org, and post future patches there as well?

    > That was my first inclination. But there is no way to create an account unlike 
    > most open source projects I work with. And I work with quite a lot.

It used to be easily possible, until someone urged students or
paid people to create such accounts and create crap bug reports.
For a while, we tried a few measures to deal with that but we
found no measure which was both fast and relatively foolproof.

--> See https://www.r-project.org/bugs.html and look for  "abuse".

I have now created an account for you.

    >> In examples like the one below, if you have R code that shows symptoms,
    >> it would really help in the bug report. 


    > I am hoping that we can look at the code as seasoned programmers and say yeah, 
    > that is a bug.

I agree in this case.
OTOH, it is exactly one of the case where the bug is not
triggerable currently:

  al <- formals(ls); length(al) <- 3

would trigger the bug... but you get an error message ".. vector .."
and as I now found that is from a slightly misguided check:
isVectorizable()  is not approriate here and should really be
replaced by isList().
  
So .. indeed, your report will have triggered an improvement in
the code, which I'm about to commit.

Thank you very much Steve!

    > I run the code through Coverity and have quite a lot of 
    > problems to tell you about.

I'm not the expert on static code analysis, but as a seasoned
statistician (*and* from experience with other such analyses) I
know that you always get false positives.

    > I run these 5 out as tests to see how this 
    > community works. I am new to this community but not necessarily R and just 
    > want to contribute back to something I am using. But believe me, I have a 
    > bunch more that seasoned programmers can eyeball and say yep - that's a bug.

Good, looking forward to see them.

    >> Otherwise, someone else will have to analyze the code to decide whether it's
    >> a bug or missing comment.  That takes time, and if there are no known
    >> symptoms, it's likely to be assigned a low priority.  The sad truth is that
    >> very few members of R Core are currently actively fixing bugs.

    > That's a shame. I'd be happy to give the scan to people in core so they can 
    > see what the lay of the land looks like.

 (hmm... the above does look a teeny tiny bit arrogant in my
  eyes; but then I'm not a native English (nor "American" :-)
  speaker ...)


    > R works amazingly good. So much so I decided to dig
    > deeper. I'd recommend to the core developers that they ask
    > to get on Coverity's open source scan list.

    > https://scan.coverity.com/

    > It's free to open source projects like this. :-)

    > -Steve


    >> On 20/07/2017 5:02 PM, Steve Grubb wrote:
    >> > Hello,
    >> > 
    >> > There appears to be a break missing in the switch/case for the LISTSXP
    >> > case. If this is supposed to fall through, I'd suggest a comment so that
    >> > others know its by design.
    >> > 
    >> > Signed-off-by: Steve Grubb <sgrubb at redhat.com>
    >> > 
    >> > Index: src/main/builtin.c
    >> > ===================================================================
    >> > --- src/main/builtin.c	(revision 72935)
    >> > +++ src/main/builtin.c	(working copy)
    >> > @@ -888,6 +888,7 @@
    >> > 
    >> >  	    SETCAR(t, CAR(x));
    >> >  	    SET_TAG(t, TAG(x));
    >> >  	
    >> >  	}
    >> > 
    >> > +	break;
    >> > 
    >> >      case VECSXP:
    >> >  	for (i = 0; i < len; i++)
    >> >  	
    >> >  	    if (i < lenx) {
    >> > 
    >> > ______________________________________________
    >> > R-devel at r-project.org mailing list
    >> > https://stat.ethz.ch/mailman/listinfo/r-devel

    > ______________________________________________
    > R-devel at r-project.org mailing list
    > https://stat.ethz.ch/mailman/listinfo/r-devel



[Rd] [PATCH] Fix missing break
Martin Maechler