Mode attribute not in clang AST.

I was wondering why the gnu "mode" attribute, even though being parsed by clang and affecting the AST construction, is not otherwise recorded in its own AST node:

def Mode : Attr {
   let Spellings = [GNU<"mode">, CXX11<"gnu", "mode">];
   let Args = [IdentifierArgument<"Mode">];
   let ASTNode = 0;
}

Other attributes having ASTNode = 0 are typically those corresponding to AttributedType nodes, but "mode" is not modeled as an AttributedType.

Would it be OK to remove the line "let ASTNode = 0" above and modify Sema::handleModeAttr to actually build the node?

Enea.

Just so I’m clear: I think you’re suggesting modeling ‘mode’ as a Decl attribute, and not as an AttributedType (or similar). That seems reasonable given the semantics of the ‘mode’ attribute. However, there is an interacting problem here: the ‘mode’ attribude currently causes us to discard type source info for a typedef to which it is applied, because (1) we have no type-level representation of the attribute, and (2) for a typedef, there is no mechanism for the underlying type to differ from the type implied by the type source info. There seem to be two natural solutions to that problem: a type-level representation of the ‘mode’ attribute (using AttributedType or similar), or allowing typedefs to have separate type and type source info. I think it makes sense to have an idea of how to solve both of these problems before attacking either of them, since their solutions interact.

    I was wondering why the gnu "mode" attribute, even though being
    parsed by clang and affecting the AST construction, is not otherwise
    recorded in its own AST node:

    def Mode : Attr {
       let Spellings = [GNU<"mode">, CXX11<"gnu", "mode">];
       let Args = [IdentifierArgument<"Mode">];
       let ASTNode = 0;
    }

    Other attributes having ASTNode = 0 are typically those
    corresponding to AttributedType nodes, but "mode" is not modeled as
    an AttributedType.

    Would it be OK to remove the line "let ASTNode = 0" above and modify
    Sema::handleModeAttr to actually build the node?

Just so I'm clear: I think you're suggesting modeling 'mode' as a Decl
attribute, and not as an AttributedType (or similar). That seems
reasonable given the semantics of the 'mode' attribute.

Correct, the proposal was in this direction.

However, there
is an interacting problem here: the 'mode' attribude currently causes us
to discard type source info for a typedef to which it is applied,
because (1) we have no type-level representation of the attribute, and
(2) for a typedef, there is no mechanism for the underlying type to
differ from the type implied by the type source info. There seem to be
two natural solutions to that problem: a type-level representation of
the 'mode' attribute (using AttributedType or similar), or allowing
typedefs to have separate type and type source info. I think it makes
sense to have an idea of how to solve both of these problems before
attacking either of them, since their solutions interact.

If we stick to treating the mode attribute as a Decl attribute, then having separate Type and TypeSourceInfo fields in a TypedefNameDecl node is probably the simplest solution. As a matter of fact, this also matches what is now happening for ValueDecl nodes: getTypeSourceInfo() and getType() will return the type information before and after the application of the attribute, respectively.

Regarding implementation:

> (2) for a typedef, there is no mechanism for the underlying type to
> differ from the type implied by the type source info.

A TypedefNameDecl node also inherits (from TypeDecl) a Type pointer field:

   /// TypeForDecl - This indicates the Type object that represents
   /// this TypeDecl. It is a cache maintained by
   /// ASTContext::getTypedefType, ASTContext::getTagDeclType, and
   /// ASTContext::getTemplateTypeParmType, and TemplateTypeParmDecl.
   mutable const Type *TypeForDecl;

Therefore, an option (having no memory space overhead) would be to build a TypedefNameDecl node where:

   - the TypeSourceInfo field points to the underlying type as written
     (i.e., before the application of mode attribute);

   - the TypeForDecl node points to the "semantic" typedef type
     (i.e., after the application of mode)

Clearly, the implementation of
   QualType getUnderlyingType() const {
     return TInfo->getType();
   }
should be changed to query the TypeForDecl field, rather than the TInfo field.

Are there reasons preventing such an approach?

Enea.

[...]

Therefore, an option (having no memory space overhead) would be to build
a TypedefNameDecl node where:

[...]

   - the TypeForDecl node points to the "semantic" typedef type
     (i.e., after the application of mode)

[...]

Are there reasons preventing such an approach?

After checking details, this can not work as proposed ...
since a TypedefType is just a pointer to the corresponding TypedefNameDecl.

Enea.

[...]

Just so I'm clear: I think you're suggesting modeling 'mode' as a Decl
attribute, and not as an AttributedType (or similar). That seems
reasonable given the semantics of the 'mode' attribute. However, there
is an interacting problem here: the 'mode' attribude currently causes us
to discard type source info for a typedef to which it is applied,
because (1) we have no type-level representation of the attribute, and
(2) for a typedef, there is no mechanism for the underlying type to
differ from the type implied by the type source info. There seem to be
two natural solutions to that problem: a type-level representation of
the 'mode' attribute (using AttributedType or similar), or allowing
typedefs to have separate type and type source info. I think it makes
sense to have an idea of how to solve both of these problems before
attacking either of them, since their solutions interact.

Please find attached a patch for review.

The approach is to:
  - model mode attributes as declaration attributes;
  - separate type and type source info in TypedefNameDecl nodes:
    this only occurs when needed, i.e., when the decl node has
    been "moded" (no memory overhead in the common case);
  - in the "moded" case, the type source info is left untouched,
    but getUnderlyingType() will return the type after mode application,
    so that everything behaves as before.

The original type source info is used when pretty printing the TypedefNameDecl node (together with the mode attribute).
The patch adds a couple of tests for the pretty printer.

For the pretty printing of VarDecl nodes:

- QualType T = D->getASTContext().getUnqualifiedObjCPointerType(D->getType());
- if (ParmVarDecl *Parm = dyn_cast<ParmVarDecl>(D))
- T = Parm->getOriginalType();
+ QualType T = D->getTypeSourceInfo()
+ ? D->getTypeSourceInfo()->getType()
+ : D->getASTContext().getUnqualifiedObjCPointerType(D->getType());
    T.print(Out, Policy, D->getName());

Doubt is the following: should we apply getUnqualifiedObjCPointerType even when considering the type source info case?

Enea.

ModeAttr.patch (7.01 KB)

Ping.

Attached a slightly reviewed patch (only changes are in test cases, where c++11 syntax has been moved into more appropriate place and suitably quoted when checking it).

OK to commit?

Enea.

ModeAttr.patch (7.1 KB)

For your question about the getUnqualifiedObjCPointerType, I don't think
so. It's probably worth adding a test case for pretty-printing "id x;
__strong id y;" in -fobjc-arc mode; we should print the __strong qualifier
for the second one, but not the first.

Otherwise, looks fine.

-Eli

Committed in r184417
(with additional ObjC pretty print test as suggested).

Thanks for reviewing,
Enea.