[PATCH] D130123: Extend ptr32 support to be applied on typedef

[Ariel Burton via Phabricator via cfe-commits]

Ariel-Burton added inline comments.


================
Comment at: clang/lib/Sema/SemaType.cpp:7116
+  for (;;) {
+    if (const TypedefType *TT = dyn_cast<TypedefType>(Desugared)) {
+      Desugared = TT->desugar();
----------------
rnk wrote:
> Ariel-Burton wrote:
> > rnk wrote:
> > > This seems like a good place to use getSingleStepDesugaredType to look through all type sugar (parens, typedefs, template substitutions, etc).
> > > This seems like a good place to use getSingleStepDesugaredType to look through all type sugar (parens, typedefs, template substitutions, etc).
> > 
> > I'm not sure what you mean.  Could you expand a little, please?
> Clang's AST has lots of "type sugar nodes". These are types which usually don't have any semantic meaning, they just carry source location information, like whether there was a typedef or extra parens in the type. AttributedType is also a type sugar node, so we cannot do a full desugaring here, we have to step through each node one at a time to accumulate the attributes.
> 
> Your code looks through one kind of type sugar, but this loop should probably be generalized to handle all kinds of type sugar. I think getSingleStepDesugaredType will do that.
Thanks for the clarification.  Do you mean something like:

 
```
 for (;;) {
    const AttributedType *AT = dyn_cast<AttributedType>(Desugared);
    if (AT) {
      Attrs[AT->getAttrKind()] = true;
      Desugared = AT->getModifiedType();
    } else {
      QualType QT = Desugared.getSingleStepDesugaredType(S.Context);
      if (Desugared != QT) {
        Desugared = QT;
      } else {
        break;
      }
    }
  }
```

I don't think that we can use getSingleStepDesugaredType indiscriminately.  Not all sugar should be peeled away, for example, in the case of of parentheses:


```
int *(__ptr32 wrong);
```

is accepted when it shouldn't.

To check for the cases where we don't want to desugar has the same sort of complexity as checking for the cases where we do.  I suggest going with the original proposal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130123