[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/ASTContext.h:2882
+
+  QualType getCorrespondingSaturatedType(const QualType &Ty) const;
 };
----------------
This probably belongs up near the other predicates.

Also I think it's more common to simply pass `QualType` instead of a `const QualType&`.


================
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);
----------------
leonardchan wrote:
> 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.
That sounds reasonable. I have no strong opinions on it and I don't think we use the functionality anyway.


================
Comment at: lib/Sema/SemaType.cpp:1612
+  // Only fixed point types can be saturated
+  if (DS.isTypeSpecSat()) {
+    if (!IsFixedPointType) {
----------------
The braces aren't needed.


Repository:
  rC Clang

https://reviews.llvm.org/D46911