On Fri, Jan 11, 2013 at 11:32 AM, jahanian <span dir="ltr"><<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div class="im"><div>On Jan 10, 2013, at 6:39 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div><br></div><blockquote type="cite">
<div class="im">On Thu, Jan 10, 2013 at 12:31 PM, jahanian <<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</a>> wrote:<br></div><blockquote type="cite"><div class="im"><br>On Jan 9, 2013, at 5:58 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
<br><blockquote type="cite"><br></blockquote><br></div><div class="im">I want to issue warning when case value overflows. EvaluateKnownConstInt detects<br>this by returning a note and callers are expected to issue the warning based on the note returned.<br>
Similar case is handled at end of CheckConvertedConstantExpression.<br></div></blockquote><div class="im"><br>Well, in CheckConvertedConstantExpression, a diagnostic is produced<br>any time the evaluation produces notes. In this case, we only issue a<br>
diagnostic for one of the notes. I'm concerned that this code would be<br>broken if we split that note into multiple notes with more details for<br>some cases -- and, in fact, there already is such a separate note for<br>
signed left shift overflow.<br></div></blockquote><div><br></div>There are many notes. But there is one note_constexpr_overflow for the overflows.</div><div>Are you suggesting something different than checking for this note?</div>
<div> (Maybe you are suggesting that we separately check for overflow condition only?).</div></div></blockquote><div><br></div><div>There are two different kinds of overflow that can happen in a case expression:</div><div>
<br></div><div>1) An overflow occurs while evaluating the case value:</div><div><br></div><div>switch (n)</div><div>  case 123456 * 123456:</div><div><br></div><div>2) An overflow occurs while converting the evaluated case value to the promoted type of the switch expression:</div>
<div><br></div><div>int n = /*...*/;</div><div>switch (n)</div><div>  case 1ULL << 45:</div><div><br></div><div>For situation (1), it doesn't seem pertinent that this is happening within a case expression; we should warn no matter where it happens; this is the focus of PR2729. (2) should already be being checked by the call to ConvertIntegerToTypeWarnOnOverflow.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="im"><blockquote type="cite">Have you thought about always warning when an expression contains an<br>
overflowing calculation, not only when such an overflow appears in a<br>case expression (like we already do for left shifts, for instance)?<br></blockquote><div><br></div></div>Are you suggesting moving the warning to individual operations;  <span style="font-family:Menlo;font-size:11px">CheckAdditionOperands,</span></div>
<div><div style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(49,89,93)">CheckMultiplyDivideOperands and the like? left shift diagnostic</div><div style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(49,89,93)">
is done in CheckShiftOperands.</div></div></div></blockquote><div><br></div><div>Yes, that would make sense to me, although we will need some mechanism to suppress the warning in the case where the overflow produces an error (as happens in constant expressions in C++11), and we'd need to avoid repeatedly evaluating subexpressions.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div class="im"><div><blockquote type="cite"><blockquote type="cite">Also, I changed the warning per your other comments.<br>
In r172102.<br></blockquote><br>Thanks! The "results in a new value" wording seems a bit awkward, how<br>about just saying "overflow in case constant expression results in<br>value %0"?</blockquote></div>
</div></div></blockquote></div>