>>>>> Matthew Fowles Kulukundis <matt.fow...@gmail.com>
>>>>> on Thu, 7 Apr 2016 11:21:56 -0400 writes:
> 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.
Thank you, Matt, for the extra context... indeed the sloppyness was not a
big deal..
> 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.
I see. But note that you misunderstood (probably my bad
English):
I did *NOT* find a test for it (and showed a case that did not
work as test, as it does not trigger an error [remember the
macro is CHECK_..() and actually shouled produce a useful error
message in a -- yet to find -- test case.
> 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.
Thank you!
At the moment, I'm too overloaed with other duties / todos.
But it is good, to have this available, and other R-devel
readers may want to help the R core team by finding and patching
(or at least reporting) such bugs.
Martin
> Cheers,
> Matt
> On Thu, Apr 7, 2016 at 6:46 AM, Martin Maechler
<maech...@stat.math.ethz.ch>
> wrote:
>> >>>>> Matthew Fowles Kulukundis <matt.fow...@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@r-project.org mailing list
>> > https://stat.ethz.ch/mailman/listinfo/r-devel
>>
> [[alternative HTML version deleted]]
______________________________________________
R-devel@r-project.org mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
