[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:206
 
+def err_invalid_saturation_spec : Error<"'%0' cannot be saturated. Only _Fract and _Accum can.">;
 def err_invalid_sign_spec : Error<"'%0' cannot be signed or unsigned">;
----------------
This error should be reworded.

Perhaps `'_Sat' specifier is only valid on '_Fract' or '_Accum', not '%0'`.


================
Comment at: lib/Sema/DeclSpec.cpp:1139
 
+  bool is_fixed_point_type =
+      (TypeSpecType == TST_accum || TypeSpecType == TST_fract);
----------------
This variable name should probably be a different case.


================
Comment at: lib/Sema/SemaType.cpp:1430
     } else {
-      switch (DS.getTypeSpecWidth()) {
-        case DeclSpec::TSW_short:
-          Result = Context.UnsignedShortAccumTy;
-          break;
-        case DeclSpec::TSW_unspecified:
-          Result = Context.UnsignedAccumTy;
-          break;
-        case DeclSpec::TSW_long:
-          Result = Context.UnsignedLongAccumTy;
-          break;
-        case DeclSpec::TSW_longlong:
-          // TODO: Replace with diag
-          llvm_unreachable("Unable to specify long long as _Accum width");
-          break;
+      if (DS.getTypeSpecSign() != DeclSpec::TSS_unsigned) {
+        switch (DS.getTypeSpecWidth()) {
----------------
You should probably use the 'getCorrespondingSaturatingType' function instead of disambiguating the whole thing here again.


================
Comment at: test/Frontend/fixed_point.c:45
+_Sat _Fract sat_fract;
+_Sat long _Fract sat_long_fract;
+
----------------
What about typedefs with the _Sat specifier on them?


Repository:
  rC Clang

https://reviews.llvm.org/D46911