[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/AST/Type.h:6551
+
+QualType getCorrespondingSaturatedType(const ASTContext &Context,
+                                       const QualType &Ty);
----------------
These should probably be in ASTContext directly.


================
Comment at: include/clang/Basic/TargetInfo.h:83
+  unsigned char LongFractWidth, LongFractAlign;
+  unsigned char SatShortAccumWidth, SatShortAccumAlign;
+  unsigned char SatAccumWidth, SatAccumAlign;
----------------
I don't think the saturating types need separate configurations. Embedded-C says "Each saturating fixed-point type has the same representation and the same rank as its corresponding primary fixed-point type."


================
Comment at: lib/Parse/ParseDecl.cpp:3614
+      } else {
+        isInvalid = DS.SetTypeSpecSat(/*isSat=*/true, Loc, PrevSpec, DiagID);
+      }
----------------
Is there a use for the isSat parameter?


================
Comment at: lib/Sema/DeclSpec.cpp:1123
+    if (!(TypeSpecType == TST_accum || TypeSpecType == TST_fract)) {
+      S.Diag(TSSatLoc, diag::err_invalid_saturation_spec)
+          << getSpecifierName((TST)TypeSpecType, Policy);
----------------
Handling this case here means that placing _Sat on something other than exactly a fixed-point type is a parsing error rather than a semantic error. How does this handle _Sat on sugared types? Should _Sat on things like typedefs work?

  typedef _Fract myfract;
  _Sat myfract F;

The primary issue (and this is one that we have encountered as well) is that you cannot have a true _Sat typedef since _Sat only exists as part of builtin types. You need to desugar/canonicalize the type and then do getCorrespondingSaturatingType (or have getCorrespondingSaturatingType look at the canonical type internally).


================
Comment at: lib/Sema/DeclSpec.cpp:1135
              TypeSpecType != TST_char && TypeSpecType != TST_wchar &&
-             TypeSpecType != TST_accum) {
+             TypeSpecType != TST_accum && TypeSpecType != TST_fract) {
       S.Diag(TSSLoc, diag::err_invalid_sign_spec)
----------------
IsFixedPointType can be used here as well.


================
Comment at: lib/Sema/DeclSpec.cpp:1165
     else if (TypeSpecType != TST_int && TypeSpecType != TST_double &&
-             TypeSpecType != TST_accum) {
+             TypeSpecType != TST_accum && TypeSpecType != TST_fract) {
       S.Diag(TSWRange.getBegin(), diag::err_invalid_width_spec)
----------------
IsFixedPointType?


================
Comment at: lib/Sema/SemaType.cpp:1410
+
+    if (DS.getTypeSpecSign() != DeclSpec::TSS_unsigned)
+      Result = fixedpoint::getCorrespondingSignedType(Context, Result);
----------------
The logic is a bit reversed. The default should be to select the signed variant, and if the TSS is unsigned, then convert it to the unsigned variant.


================
Comment at: lib/Sema/SemaType.cpp:1609
     declarator.setInvalidType(true);
 
   // Handle complex types.
----------------
Other qualifiers like const and volatile are handled down here. Should the _Sat application be performed somewhere here instead?


Repository:
  rC Clang

https://reviews.llvm.org/D46911