[PATCH] D51329: [Attribute/Diagnostics] Print macro instead of whole attribute for address_space

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/AST/Type.h:4183
+/// Sugar type that represents a type that was defined in a macro.
+class MacroDefinedType : public Type {
+  friend class ASTContext; // ASTContext creates these.
----------------
This represents a macro qualifier, not a type defined in a macro, so I don't think this name is appropriate. Maybe `MacroQualifiedType`?


================
Comment at: clang/include/clang/AST/Type.h:4199
+  const IdentifierInfo *getMacroIdentifier() const { return MacroII; }
+  QualType getUnderlyingType() const { return UnderlyingTy; }
+
----------------
Please also provide an accessor to get the unqualified type here (that is, the type that we would have had if the macro had not been written -- the original type of the `AttributedType`). Right now, that's (effectively) being computed in the type printer, which doesn't seem like the right place for that logic.


================
Comment at: clang/lib/AST/Type.cpp:382-386
+QualType QualType::IgnoreMacroDefinitions(QualType T) {
+  while (const auto *MDT = T->getAs<MacroDefinedType>())
+    T = MDT->getUnderlyingType();
+  return T;
+}
----------------
This doesn't seem like a sufficiently general operation to warrant being a member of `QualType` to me, and like `IgnoreParens` before it, this is wrong with regard to qualifiers (it drops top-level qualifiers on the type if they're outside the macro qualifier). Looking through the uses below, I think there are better ways to write them that avoid the need for this function.


================
Comment at: clang/lib/AST/TypePrinter.cpp:969-970
+                                          raw_ostream &OS) {
+  if (const auto *AttrTy = dyn_cast<AttributedType>(T->getUnderlyingType())) {
+    if (AttrTy->getAttrKind() == attr::AddressSpace) {
+      OS << T->getMacroIdentifier()->getName() << " ";
----------------
This should apply to all attributes, not only address space attributes.


================
Comment at: clang/lib/AST/TypePrinter.cpp:975-979
+      SplitQualType SplitTy = AttrTy->getModifiedType().split();
+      Qualifiers Quals = SplitTy.Quals;
+      if (Quals.getAddressSpace() >= LangAS::FirstTargetAddressSpace)
+        Quals.removeAddressSpace();
+      return printBefore(SplitTy.Ty, Quals, OS);
----------------
This is the code that I mentioned above that should be moved to `MacroDefinedType`.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:252
+
+    // If this was declared in a macro, attatch the macro IdentifierInfo to the
+    // parsed attribute.
----------------
Typo "attatch"


================
Comment at: clang/lib/Parse/ParseDecl.cpp:257-262
+      bool AttrStartIsInMacro =
+          (StartLoc.isMacroID() && Lexer::isAtStartOfMacroExpansion(
+                                       StartLoc, SrcMgr, PP.getLangOpts()));
+      bool AttrEndIsInMacro =
+          (EndLoc.isMacroID() &&
+           Lexer::isAtEndOfMacroExpansion(EndLoc, SrcMgr, PP.getLangOpts()));
----------------
I think these checks need to be moved to the last loop of `FindLocsWithCommonFileID`. Otherwise for a case like:

```
#define THING \
  int *p = nullptr;
  AS1 int *q = nullptr;

THING
```

... `FindLocsWithCommonFileID` will pick out the `THING` macro and return the range from the `int` token to the second semicolon, and the checks here will fail. Instead, we should pick out the inner `AS1` expansion, because it's the outermost macro expansion that starts with `StartLoc` and ends with `EndLoc`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:13667
+                               .IgnoreMacroDefinitions()
+                               .getAsAdjusted<FunctionProtoTypeLoc>())) {
 
----------------
Please change `TypeLoc::getAsAdjusted` to skip macro type sugar instead of skipping it here. (`getAsAdjusted` already skips `AttributedType` sugar, and your new sugar node is the same kind of thing.)

Please also change `Type::getAsAdjusted` to likewise remove the new type node.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3389-3395
+  TypeLoc TL = FD->getTypeSourceInfo()
+                   ->getTypeLoc()
+                   .IgnoreParens()
+                   .IgnoreMacroDefinitions();
   while (auto ATL = TL.getAs<AttributedTypeLoc>())
-    TL = ATL.getModifiedLoc().IgnoreParens();
+    TL = ATL.getModifiedLoc().IgnoreParens().IgnoreMacroDefinitions();
   return TL.castAs<FunctionProtoTypeLoc>().getReturnLoc();
----------------
This duplicates the logic in `TypeLoc::getAsAdjusted` (and misses a case!). This whole function definition should be replaced by `return FD->getTypeSourceInfo()->getTypeLoc().getAsAdjusted<FunctionProtoTypeLoc>().getReturnLoc();`.


================
Comment at: clang/lib/Sema/SemaType.cpp:6967-6972
+  QualType R = T.IgnoreParens().IgnoreMacroDefinitions();
   while (const AttributedType *AT = dyn_cast<AttributedType>(R)) {
     if (AT->isCallingConv())
       return true;
-    R = AT->getModifiedType().IgnoreParens();
+    R = AT->getModifiedType().IgnoreParens().IgnoreMacroDefinitions();
   }
----------------
Instead of calling `IgnoreParens()` and `IgnoreMacroDefinitions` here, I think we should just be ignoring any type sugar by using `getAs<AttributedType>()`:

```
bool Sema::hasExplicitCallingConv(QualType T) {
  while (const auto *AT = T->getAs<AttributedType>()) {
    if (AT->isCallingConv())
      return true;
    T = AT->getModifiedType();
  }
  return false;
}
```

(Note also the change from passing `T` by reference -- and then never storing to it in the function -- to passing it by value.)


================
Comment at: clang/lib/Sema/SemaType.cpp:7553-7560
+    // Handle attributes that are defined in a macro.
+    if (attr.hasMacroIdentifier() && !type.getQualifiers().hasObjCLifetime()) {
+      const IdentifierInfo *MacroII = attr.getMacroIdentifier();
+      if (FoundMacros.find(MacroII->getName()) == FoundMacros.end()) {
+        type = state.getSema().Context.getMacroDefinedType(type, MacroII);
+        FoundMacros.insert(MacroII->getName());
+      }
----------------
The handling (both here and in the parser) for a macro that defines multiple attributes doesn't seem right. Given:

```
#define ATTRS __attribute__((attr1, attr2))
ATTRS int x;
```

it looks like you'll end up with a type built as

```
AttributedType attr1
 `- MacroDefinedType
      `- AttributedType attr2
          `- int
```

... which seems wrong.


The parser representation also can't distinguish between the above case and

```
#define ATTRS __attribute__((attr1))
ATTRS
#define ATTRS __attribute__((attr2))
ATTRS
int x;
```

... since both will be represented as simply a macro identifier `ATTR` attached to two different attributes (you could solve this by tracking the source location of the expansion loc of the macro in the parsed attribute representation, instead of or in addition to the macro identifier).

And the AST representation can't distinguish between the above and

```
#define ATTRS __attribute__((attr1))
ATTRS __attribute__((attr2)) int x;
```

... because it doesn't track what the modified type was, so we have to guess which attributes are covered by the macro.

Perhaps the best thing would be to support only the case of a macro that expands to exactly one attribute for the initial commit, and add support for macros covering multiple attributes as a follow-up commit?


================
Comment at: clang/test/Frontend/macro_defined_type.cpp:9
+
+  // There should be no difference wether a macro defined type is used or not.
+  auto __attribute__((noderef)) *auto_i_ptr = i_ptr;
----------------
Typo 'wether'


Repository:
  rC Clang

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

https://reviews.llvm.org/D51329