leonardchan added inline comments.
================
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;
+}
----------------
rsmith wrote:
> 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.
Removed this method.
================
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()));
----------------
rsmith wrote:
> 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`.
Moved, although this doesn't appear to fix this case. On closer inspection,
when generating the vector of starting locations, the expansion location for
`AS1` doesn't seem to be returned by `getExpansionLocStart`. It goes straight
from the location of the `__attribute__` behind `AS1` to `THING` and skips
`AS1`. I found this out by just dumping `StartLoc` on every iteration.
The only way I can generate the location for `AS1` in `THING` is by also
watching for the spelling location of each expansion, but this SourceLoc is not
a macro ID and would not fulfill these last 2 checks. Is this intended? If it's
a bug I don't mind addressing it if you point me in the right direction to
start.
================
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();
}
----------------
rsmith wrote:
> 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.)
When using only `getAs`, the following 3 tests fail:
Clang :: CodeGenCXX/mangle-ms.cpp
Clang :: SemaCXX/calling-conv-compat.cpp
Clang :: SemaCXX/decl-microsoft-call-conv.cpp
since using `getAs` would involved removing a `TypeDefType` for each of these.
Looking at the comment above this method's declaration, part of the intent is
to retain the typedef sugar. We could still use `getAs` and not
`IgnoreMacroDefinitions` by additionally checking for a typedef:
```
const AttributedType *AT;
while ((AT = T->getAs<AttributedType>()) && !isa<TypedefType>(T)) {
```
Using `!T->getAs<TypedefType>()` will still fail for types laid out as:
```
AttributedType
` - TypedefType
```
================
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());
+ }
----------------
rsmith wrote:
> 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?
If it makes this cleaner and easier to review, I don't mind separating it into
2 patches with the first working for exactly one and the followup for multiple.
For us though, we would mainly be using this for multiple attribute macros
(especially `__attribute__((address_space(n), noderef))`. I can split it into 2
if you would like after this revision.
As for the cases you presented, for
```
#define ATTRS __attribute__((attr1, attr2))
ATTRS int x;
```
I see that this prevents from adding any more `MacroQualifiedType`s for
attributes that share the same name. Updated in this revision so that we
continue to wrap `AttributedType`s, producing:
```
MacroQualifiedType (ATTRS)
`- AttributedType attr1
` - MacroQualifiedType (ATTRS)
`- AttributedType attr2
`- int
```
In the case of
```
#define ATTRS __attribute__((attr1))
ATTRS
#define ATTRS __attribute__((attr2))
ATTRS
int x;
```
I added the expansion loc so we can distinguish between this and the first case
now.
As for
```
#define ATTRS __attribute__((attr1))
ATTRS __attribute__((attr2)) int x;
```
With this new revision, the AST representation now differs from the previous 2
examples in that only `attr1` gets wrapped with a `MacroQualifiedType` and not
`attr2` producing:
```
AttributedType attr2
` - MacroQualifiedType (ATTRS)
`- AttributedType attr1
`- int
```
I also added test cases for these.
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
