r183084 - Properly consider the range of enum for range comparisons in C mode

[John McCall]

On Jun 7, 2013, at 11:35 AM, David Majnemer <david.majnemer at gmail.com> wrote:
> On Friday, June 7, 2013, John McCall wrote:
> On Jun 2, 2013, at 1:11 AM, David Majnemer <david.majnemer at gmail.com> wrote:
> > Author: majnemer
> > Date: Sun Jun  2 03:11:22 2013
> > New Revision: 183084
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=183084&view=rev
> > Log:
> > Properly consider the range of enum for range comparisons in C mode
> >
> > In some cases, clang applies the C++ rules for computing the range of a
> > value when said value is an enum.
> >
> > Instead, apply C semantics when in C mode.
> 
> Was this proposed or reviewed somewhere?  This is totally wrong.
> 
> It was proposed on this list in the form of a patch and LGTM'd. I then waited for more comments and received none.

Ah, I see it.  I'll note that almost all of this waiting happened over the weekend, which is not exactly the peak review period, but okay, I just missed the thread.

The out-of-range-constant-compare warning should be using a conservatively-wide estimate of the range of the value, not a conservatively-narrow estimate.  For example, the analysis is currently based only on types, but if it were instead analyzing expressions, it would want to evaluate the width of adding two chars as 9 bits, not 8.

There are other warnings, like implicit-truncation warnings, for which the reverse analysis is more appropriate.  Those warnings are what most of the IntRange framework are based around.

It is reasonable for a warning that wants a conservatively-narrow analysis to expect the range of an enum to be just the range of its values.  This avoids, e.g., warnings about assigning an enum to a small bit-field.

It would be reasonable to make the direction of conservatism a parameter of IntRange::forValueOfType — in fact, it would be reasonable to make it a parameter of GetExprRange, although that would require careful inspection of all of that code.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130607/215e656d/attachment.html>