Matthew Fowles Kulukundis
2016-Apr-05 15:17 UTC
[Rd] [PATCH] fix CHECK_this_length in sprintf.c
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. Matt -------------- next part -------------- A non-text attachment was scrubbed... Name: r-sprintf.patch Type: text/x-patch Size: 758 bytes Desc: not available URL: <https://stat.ethz.ch/pipermail/r-devel/attachments/20160405/bbd3947a/attachment.bin>
>>>>> 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
Matthew Fowles Kulukundis
2016-Apr-07 15:21 UTC
[Rd] [PATCH] fix CHECK_this_length in sprintf.c
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]]