From kevinushey at gmail.com  Tue Jun 24 07:28:21 2014
From: kevinushey at gmail.com (Kevin Ushey)
Date: Mon, 23 Jun 2014 22:28:21 -0700
Subject: [Rd]
	=?utf-8?q?operation_on_=E2=80=98numsels=E2=80=99_may_be_unde?=
	=?utf-8?q?fined?=
In-Reply-To: <CAC2h7utL_vzyRCSOHB9xi+nqmkm3aqWwNBbiitvpsu4jevqr6g@mail.gmail.com>
References: <53A8298C.9010605@aon.at>
	<244EC819-4336-4A49-BAEB-86B57E8A2970@r-enthusiasts.com>
	<53A855C6.1000301@aon.at>
	<BE6B83B4-2A60-4F84-8B5F-C0754997E5C5@r-enthusiasts.com>
	<53A877EE.1080201@aon.at> <53A8A79A.9070301@fhcrc.org>
	<53A8AD3F.2030901@fhcrc.org>
	<CAC2h7utL_vzyRCSOHB9xi+nqmkm3aqWwNBbiitvpsu4jevqr6g@mail.gmail.com>
Message-ID: <CAJXgQP0racSjePV_Aoexkcr6DGCyQ_VZv7GF+SuJfGm_da6rYA@mail.gmail.com>

I don't see what's so surprising here.

That statement is identical to writing:

    if (arrMask[i] == 1) {
        numsels = ++numsels;
    } else {
        numsels = numsels;
    }

and

    numsels = ++numsels;

has two statements modifying the value of numsels (= and prefix-++) in
a single sequence point. (Do we increment then assign, or assign then
increment? The C / C++ standards leave this undefined.)

Imagine writing the operations out as functions: we have the `=`
function, and the `prefix-++` function -- both of these 'modify' (one
of) their arguments. Do we evaluate it as `=`(a, `prefix-++`(a)) or
`prefix-++`(`=`(a, a))? The C standard leaves this undefined, so
compilers are free to do what they wish (and the nice ones warn you
when there is such an ambiguity). I guess the net result of the
operation is the same in each case _here_, but this is of course not
the case for other functions that modify the value of their
operand(s). And, in truth, this is _undefined behaviour_ and so the
compiler could still rightly make demons fly out of your nose if it
wanted to upon program execution.

I highly recommend reading the slides at
http://www.slideshare.net/olvemaudal/deep-c, especially the bit on
sequence points.

Cheers,
Kevin

On Mon, Jun 23, 2014 at 9:22 PM, Kasper Daniel Hansen
<kasperdanielhansen at gmail.com> wrote:
> I am not an expert on this, but I note that the section on -Wsequence-point
> at
>   http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
> specifically mentions ? and :.  Perhaps some more work on tracking down
> their definitions and precedence might lead to insights.
>
> Best,
> Kasper
>
>
> On Mon, Jun 23, 2014 at 6:42 PM, Herv? Pag?s <hpages at fhcrc.org> wrote:
>
>> On 06/23/2014 03:18 PM, Herv? Pag?s wrote:
>>
>>> Hi Christian,
>>>
>>> On 06/23/2014 11:54 AM, cstrato wrote:
>>>
>>>> Dear Romain,
>>>>
>>>> I do not know enough about compilers, but as far as I remember, they
>>>> 'work' from right to left,
>>>>
>>>
>>> Not necessarily. So you should not rely on that. This is what the
>>> (somewhat obscure) warning you see on zin1 is trying to tell you.
>>>
>>
>> Actually, I don't see an ambiguity in your code:
>>
>>
>>   numsels = (arrMask[i] == 1) ? ++numsels : numsels;
>>
>> Yes it's confusing and unnecessarily complicated but I don't see that
>> it relies on some undefined behavior. It's not like in the thread on
>> Bioc-devel where the expression was:
>>
>>   *p++ = tolower(*p);
>>
>> In that case the left-value of the assignment is itself an expression
>> that needs to be evaluated and the outcome of the assignment depends
>> on whether the left-value is evaluated before the right expression or
>> not. But in your case the left-value is a variable name so there is
>> nothing to evaluate.
>>
>> So I don't know. Looks like a false positive from the gcc compiler to
>> me. Someone on the list might have a better insight.
>>
>> Cheers,
>>
>> H.
>>
>>
>>  Personally I would just do:
>>>
>>>      if (arrMask[i] == 1) numsels++;
>>>
>>> which is the standard way to implement the "if (some condition)
>>> then increment counter" idiom.
>>>
>>> As Kasper mentioned, a similar issue was recently discussed here:
>>>
>>>    https://stat.ethz.ch/pipermail/bioc-devel/2014-June/005858.html
>>>
>>> Cheers,
>>> H.
>>>
>>>
>>>  so numsels = ++numsels should not confuse the
>>>> compiler. Anyhow I will change my code to your first suggestion since it
>>>> is more elegant.
>>>>
>>>> Best regards,
>>>> Christian
>>>>
>>>>
>>>> On 6/23/14 7:13 PM, Romain Fran?ois wrote:
>>>>
>>>>>
>>>>> Le 23 juin 2014 ? 18:28, cstrato <cstrato at aon.at> a ?crit :
>>>>>
>>>>>  Dear Romain,
>>>>>>
>>>>>> Thank you for your suggestions, I like especially the first one.
>>>>>>
>>>>>> However, you did not explain why I have never got this warning
>>>>>> message on any compiler, and why only one of the two identical Ubuntu
>>>>>> compilers did give this warning message?
>>>>>>
>>>>>> Best regards,
>>>>>> Christian
>>>>>>
>>>>>
>>>>> I don?t know, but this:
>>>>>
>>>>> numsels = ++numsels ;
>>>>>
>>>>> seems fishy to me, and so it keeps feeling weird with the addition of
>>>>> the ternary operator.
>>>>>
>>>>> There is obviously a difference of setup between these two machines,
>>>>> but I don?t have time to sherlock that for you. One of the compilers
>>>>> is getting more careful than the other. Getting warnings you did not
>>>>> get before is a good thing, as it helps you update the code with that
>>>>> new insight.
>>>>>
>>>>> Welcome to my world, I?m sometimes thrown all kinds of new warnings
>>>>> from esoteric compilers, all of them have value .
>>>>>
>>>>> Romain
>>>>>
>>>>>  On 6/23/14 3:45 PM, Romain Fran?ois wrote:
>>>>>>
>>>>>>>
>>>>>>> Le 23 juin 2014 ? 15:20, cstrato <cstrato at aon.at> a ?crit :
>>>>>>>
>>>>>>>  Dear all,
>>>>>>>>
>>>>>>>> Since many years the following C++ code does compile on ALL
>>>>>>>> Bioconductor servers (Linux, Windows, Mac) without any warnings:
>>>>>>>>
>>>>>>>>    Int_t numsels = 0;  //number of selected entries
>>>>>>>>    ...
>>>>>>>>    for (Int_t i=0; i<size; i++) {
>>>>>>>>       numsels = (arrMask[i] == 1) ? ++numsels : numsels;
>>>>>>>>    }//for_i
>>>>>>>>
>>>>>>>
>>>>>>> This is confusing. I would write the loop body like this:
>>>>>>>
>>>>>>> numsels += (arrMask[i] == 1) ;
>>>>>>>
>>>>>>>
>>>>>>> or preferably using the STL:
>>>>>>>
>>>>>>> Int_t numsels = std::count( begin(arrMask), end(arrMask), 1 ) ;
>>>>>>>
>>>>>>> or some other variation of this, i.e. perhaps you don?t have a C++11
>>>>>>> compiler, so perhaps one of these depending on what is arrMask:
>>>>>>>
>>>>>>> Int_t numsels = std::count( arrMask.begin(), arrMask.end(), 1 ) ;
>>>>>>> Int_t numsels = std::count( arrMask, arrMask + size, 1 ) ;
>>>>>>>
>>>>>>> Romain
>>>>>>>
>>>>>>>  Even on the recently added release server 'zin2' Linux (Ubuntu
>>>>>>>> 12.04.4 LTS) the above code compiles w/o warnings.
>>>>>>>>
>>>>>>>> However, on the new development server 'zin1' Linux (Ubuntu 12.04.4
>>>>>>>> LTS) I get suddenly the following warning message:
>>>>>>>>
>>>>>>>> Found the following significant warnings:
>>>>>>>>   XPSPreProcessing.cxx:3026:56: warning: operation on ?numsels? may
>>>>>>>> be undefined [-Wsequence-point]
>>>>>>>>
>>>>>>>> Interestingly, both servers do not only run the same version of
>>>>>>>> Ubuntu, but also the same version of the C++ compiler, i.e. g++
>>>>>>>> (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3, and use the same flags, see:
>>>>>>>> http://bioconductor.org/checkResults/2.14/bioc-LATEST/
>>>>>>>> zin2-NodeInfo.html
>>>>>>>>
>>>>>>>>
>>>>>>>> http://bioconductor.org/checkResults/devel/bioc-
>>>>>>>> LATEST/zin1-NodeInfo.html
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> My question is now, why do I suddenly get the compiler warning?
>>>>>>>>
>>>>>>>> The reason why I ask at R-devel and not Bioc-devel is that it may
>>>>>>>> not only be a Bioc question, since I found the following links:
>>>>>>>> http://c-faq.com/expr/seqpoints.html
>>>>>>>> http://stackoverflow.com/questions/16838884/why-i-got-
>>>>>>>> operation-may-be-undefined-in-statement-expression-in-c
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I am not sure if I understand the meaning, but until now I have
>>>>>>>> never got any warning from any compiler the I have used (including
>>>>>>>> MS Visual C++).
>>>>>>>>
>>>>>>>> Do I really have to replace '++numsels' with 'numsels+1'?
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Christian
>>>>>>>> _._._._._._._._._._._._._._._._._._
>>>>>>>> C.h.r.i.s.t.i.a.n   S.t.r.a.t.o.w.a
>>>>>>>> V.i.e.n.n.a           A.u.s.t.r.i.a
>>>>>>>> e.m.a.i.l:        cstrato at aon.at
>>>>>>>> _._._._._._._._._._._._._._._._._._
>>>>>>>>
>>>>>>>> ______________________________________________
>>>>>>>> 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
>>>>
>>>
>>>
>> --
>> Herv? Pag?s
>>
>> Program in Computational Biology
>> Division of Public Health Sciences
>> Fred Hutchinson Cancer Research Center
>> 1100 Fairview Ave. N, M1-B514
>> P.O. Box 19024
>> Seattle, WA 98109-1024
>>
>> E-mail: hpages at fhcrc.org
>> Phone:  (206) 667-5791
>> Fax:    (206) 667-1319
>>
>> ______________________________________________
>> R-devel at r-project.org mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>
>
>         [[alternative HTML version deleted]]
>
>
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>



[Rd] =?utf-8?q?operation_on_=E2=80=98numsels=E2=80=99_may_be_unde?= =?utf-8?q?fined?=
Kevin Ushey