From matt.fowles at gmail.com  Thu Apr  7 17:21:56 2016
From: matt.fowles at gmail.com (Matthew Fowles Kulukundis)
Date: Thu, 7 Apr 2016 11:21:56 -0400
Subject: [Rd] [PATCH] fix CHECK_this_length in sprintf.c
In-Reply-To: <22278.15006.871215.53326@stat.math.ethz.ch>
References: <CAApERuYP=0VZqzi-J3cB2zmZNwxhAyR_6GLacHs4zYbjzuzkKA@mail.gmail.com>
	<22278.15006.871215.53326@stat.math.ethz.ch>
Message-ID: <CAApERuYJ3zfLTc=H3LJkqV4Nx1U_s5=1iJE4hDH1iXbWRBaL3A@mail.gmail.com>

Martin~

Sorry about the bad patch.  I work on C++ at Google. We built a check for
clang-tidy that identifies errors of this form and discovered the error
here as part of our search. I am just trying to be a good citizen and
upstream a fix, but I must have gotten sloppy as I was doing a bunch of
these.

Thanks for fixing it and finding the a test for it, I actually had no idea
how to trigger this codepath and have never used R.

If you are curious, the upstream check for clang-tidy is
http://reviews.llvm.org/D18766
You may consider running some of the other clang-tidy checks on your source
base, they will likely find other bugs.

Cheers,
Matt

On Thu, Apr 7, 2016 at 6:46 AM, Martin Maechler <maechler at stat.math.ethz.ch>
wrote:

> >>>>> Matthew Fowles Kulukundis <matt.fowles at gmail.com>
> >>>>>     on Tue, 5 Apr 2016 11:17:30 -0400 writes:
>
>     > All~
>     > CHECK_this_length macro expands to multiple statements making it
> unsafe to
>     > use in a single line `if` statement (as is happening near line
> 335).  This
>     > fixes the macro using the standard `do { } while (0)` macro trick.
>
> yes, but you forgot the closing '}' ... so you could not even
> have compiled R after applying your patch.
>
> Also, it would be nice to contrive a minimal example where the
> change makes a difference.  This  "fails" to trigger :
>
> --------------------------------
> as.double.foo <- function(x) x[FALSE]
> x <- structure(3, class="foo")
> as.numeric(x) # numeric(0)
>
> sprintf("%d !", x)# "3 !"  instead of giving an error
> --------------------------------
>
> Thank you, Matt, in any case; this (with the "{" !) has now gone
> into the source.
>
> Martin
>
>     > Matt
>     > x[DELETED ATTACHMENT external: r-sprintf.patch, text/x-patch]
>     > ______________________________________________
>     > R-devel at r-project.org mailing list
>     > https://stat.ethz.ch/mailman/listinfo/r-devel
>

	[[alternative HTML version deleted]]



[Rd] [PATCH] fix CHECK_this_length in sprintf.c
Matthew Fowles Kulukundis