[PATCH] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: clang/test/Frontend/fixed_point_conversions.c:45
+
+short _Accum sa_const9 = 2u; // DEFAULT-DAG: @sa_const9 = {{.*}}global i16 256, align 2
+unsigned short _Accum usa_const3 = 2;
----------------
bjope wrote:
> Perhaps we should verify that we do not mess up signed values when using the -fpadding-on-unsigned-fixed-point option (having CHECK, DEFAULT and SAME as check prefixes)?
> 
> (nit: I also see that we now use different names for this check prefixes in different test files, which could be confusing. Why is it called SAME here?) 
> 
> 
> 
> Perhaps we should verify that we do not mess up signed values when using the -fpadding-on-unsigned-fixed-point option (having CHECK, DEFAULT and SAME as check prefixes)?

Done

> (nit: I also see that we now use different names for this check prefixes in different test files, which could be confusing. Why is it called SAME here?)

Sorry forgot to update these prefixes.


================
Comment at: clang/test/Frontend/fixed_point_conversions.c:66
 
+_Sat short _Accum sat_sa_const3 = 256;  // DEFAULT-DAG: @sat_sa_const3 = {{.*}}global i16 32767, align 2
+_Sat short _Accum sat_sa_const4 = -257; // DEFAULT-DAG: @sat_sa_const4 = {{.*}}global i16 -32768, align 2
----------------
bjope wrote:
> A little bit confusing to be with this mix of having checks left-aligned (on separate lines) and some checks appended like this.
> I can see that it already is like that before, so maybe it should be cleaned up in a separate commit (before this commit).
> 
> (I'm not so familiar with clang tests, so maybe it is common to have the FileCheck checks at the end of the line. But I've never seen that in llvm.)
Will change this, but would be easier for me if it was done after this commit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56900/new/

https://reviews.llvm.org/D56900