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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
