[PATCH] D23833: [Polly] Introduce unittests.

[Tobias Grosser via llvm-commits]

grosser added inline comments.

================
Comment at: unittests/Isl/IslTest.cpp:27
@@ +26,3 @@
+    isl_val_free(IslNOne);
+  }
+
----------------
Meinersbur wrote:
> grosser wrote:
> > 	​
> > 22		​  {
> > 23		​    APInt APNOne(32, -1, true);
> > 24		​    auto *IslNOne = isl_valFromAPInt(IslCtx, APNOne, false);
> > 
> > with 'false' in valFromAPInt might also be an interesting test case.
> I am not sure what the result even should be as the test was specifically designed to get negative one. Currently it returns '4294967295'. If this is the indented result, we should test
> ```
> APInt APNOne(32, 4294967295, false);
> auto *IslNOne = isl_valFromAPInt(IslCtx, APNOne, false);
> EXPECT_EQ(isl_val_cmp_si(IslNOne , 4294967295), 0);
> ```
> The APInt is equal to `APInt(32, -1, true)`. The third argument only matters if first argument `>64`
I wanted to test how negative one is converted if interpreted as unsigned in isl_valFromAPInt. 4294967295 is the answer I would expect. However, it is probably more readable if written as "1 << 32 - 1". As you noted APInt(32, 1 << 32 -1, false) should be equal to APInt(32, -1, true), so which of the two is used to construct the APInt probably does not matter.

If you agree such a test case is useful, I would it would be great if you could add it. Feel free to choose whichever constructor you prefer in APInt.

================
Comment at: unittests/Isl/IslTest.cpp:61
@@ +60,3 @@
+    // FIXME: The canonical representation of a negative APInt is two's
+    // complement.
+    EXPECT_EQ(APNOne, 1);
----------------
Meinersbur wrote:
> grosser wrote:
> > Very interesting observation. Do you have an idea why this is working today? Or does this indeed result in a bug that can be triggered externally?
> I don't know.
That's fair. We can work on this after the unit-test infrastructure is in place.


https://reviews.llvm.org/D23833