[llvm-commits] [patch][pr12251] Add range metadata to llvm. Make clang produce it.

[Richard Smith]

2012/3/23 Rafael Espíndola <rafael.espindola at gmail.com>

> > You should use IsFixed, as you are doing. The TypeSourceInfo* / Type*
> > distinction only exists to capture whether the type was literally
> written in
> > the source code (a scoped enumeration with no specified underlying type
> has
> > an implicit fixed underlying type of 'int').
> >
> > Have you considered s/isBooleanUnderlyingType/hasBooleanRepresentation/
> to
> > match how you've generalized it?
> >
> > Please also add some test coverage for scoped enumerations.
>
> Fixed the above comments.


Thanks.


> > Finally, this patch does the wrong thing for an enum which is empty, or
> > whose only value is 0. Such an enum has a range of representable values
> of
> > [0, 1], not [0, 0].
>
> Has this been changed recently? I am looking at paragraph 7 of
> [dcl.enum] in n3337.  I am getting
>
> * emin = emax = 0
> * K = 1
> * max(|emin| - K, |emax|) = max (-1, 0) = 0
> * M = 0
> * bmax = 2^M - 1 = 1 - 1 = 0
>

Right you are, I was thinking of the smallest corresponding bitfield size,
which is max(M, 1).

+  if (const EnumType *ET = dyn_cast<EnumType>(Ty))

+  const EnumType *ET =
dyn_cast<EnumType>(Ty.getDesugaredType(getContext()));

Please use Ty->getAs<EnumType>() in both of these cases.

+    // The range is open on the right side.
+    uint64_t End = Max + 1;

All of your non-bool Max computations end with '- 1'. It looks like the
code would be simpler if it computed End directly.

Thanks!
Richard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120323/0b1e9586/attachment.html>