On Jun 7, 2013, at 11:35 AM, David Majnemer <[email protected]> wrote:
> On Friday, June 7, 2013, John McCall wrote:
> On Jun 2, 2013, at 1:11 AM, David Majnemer <[email protected]> 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.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
