[cfe-commits] [PATCH] Improved handling of 128-bit integer literals

[Dmitri Gribenko]

On Wed, Nov 21, 2012 at 11:27 PM, Stephen Canon <scanon at apple.com> wrote:
> Hi all --
>
> This patch attempts to clean up the situation for 128-bit and MS-style integer literals, as discussed on cfe-dev back in September ("MS 128-bit literals don't always have the correct type").

Hi Stephen,

Thank you for working on this!

-    "  1i128, -1i128, 1ui128, 1Ui128,"
-    "  0x10000000000000000i128;"
+    "  1i64, -1i64, 1ui64;"

Could you please add at least one signed and one unsigned 128-bit
literal here (in new syntax) to test StmtPrinter?

+    if (!ArgNum.isIntN(64)) {
+      S.Diag(Attr.getLoc(), diag::err_attribute_argument_out_of_bounds)
+      << "alloc_size" << I.getArgNum() << Ex->getSourceRange();
+      return;
+    }

There are similar issues in 'nonnull', 'ownership' and 'format'
attributes.  I have an incomplete patch for all these, that refactors
duplicated code over handle*() functions in SemaDeclAttr.cpp and fixes
this 128-bit issue, but I decided not to submit it until the semantic
analysis is fixed.

+      // Handle Microsoft literals entirely separately, as the behave very
+      // differently from the suffixes specified by the C and C++ standards.

Typo: the -> they

+      // Handle Microsoft literals entirely separately, as the behave very
+      // differently from the suffixes specified by the C and C++ standards.

Could you also change add "(i8, i16, i32, i64 suffixes)" after
"Microsoft literals" for clarification?

+      if (!Literal.isUnsigned && !Literal.isLong && !Literal.isLongLong) {
+        Ty = Context.IntTy;
+        Width = Context.getTargetInfo().getIntWidth();
+      } else if (!Literal.isLong && !Literal.isLongLong) {
[...]
+      if (ResultVal.getBitWidth() != Width)
+        ResultVal = ResultVal.trunc(Width);

Do we have tests for this?  If not, could you please add tests?  (It
should be doable with some template metaprogramming magic).

+    }
+
+    else {

Should be '} else {'

+        // rules of C11 6.4.4.1 and C++11 2.14.2.  Those rules can be

Please use section names where possible, for example the second
reference should be C++11 [lex.icon].

+        bool FitsInLongLong = FitsInUnsignedLongLong &&
+        ResultVal[LongLongWidth-1] == 0;

Indentation is off here.

I see that the code is much cleaner when FitsIn* are computed upfront,
but this leads to some extra work -- each isIntN() boils down to
counting leading zeros.  Is there a clean way to defer the computation
to the point where it is required?  I don't know how hot this code is,
so maybe this is not worth doing.

+        }
+        else {

Should be '} else {'

This LGTM with tests and code style changes mentioned above, but
please wait for Richard Smith's review.

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/