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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
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);
----------------
ebevhan wrote:
> 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).
I think _Sat is analogous to _Complex where it only works with specific builtin types, albeit the only builtin type _Complex doesn't work with is _Bool.

Currently this example would throw the error `'_Sat' specifier is only valid on '_Fract' or '_Accum', not 'type-name'` which is similar to what _Complex does when paired with a typedef:

```
typedef double mydouble;
mydouble _Complex D;  // _Complex type-name' is invalid
```

I don't see this as a big problem right now, but am willing to come back to this in the future if it becomes more urgent. For now, I added a test that asserts this error is thrown.


================
Comment at: lib/Sema/SemaType.cpp:1410
+
+    if (DS.getTypeSpecSign() != DeclSpec::TSS_unsigned)
+      Result = fixedpoint::getCorrespondingSignedType(Context, Result);
----------------
ebevhan wrote:
> 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.
Using getCorrespondingUnsignedType(). Also changed getCorrespondingUnsignedType() to accept fixed point types.


Repository:
  rC Clang

https://reviews.llvm.org/D46911