rsmith added inline comments.
Herald added a subscriber: rnkovacs.
================
Comment at: clang/include/clang/AST/Type.h:4174
+/// Sugar type that represents a type that was defined in a macro.
+class MacroQualifiedType : public Type {
----------------
This comment is out of date. "[...] a type that was qualified by a qualifier
written as a macro invocation" maybe?
================
Comment at: clang/include/clang/AST/Type.h:4183
+ MacroQualifiedType(QualType UnderlyingTy, QualType CanonTy,
+ const IdentifierInfo *MacroII, SourceLocation
ExpansionLoc)
+ : Type(MacroQualified, CanonTy, UnderlyingTy->isDependentType(),
----------------
Types should not carry source locations. If you want location information, it
should be stored on the `TypeLoc` instead.
================
Comment at: clang/include/clang/AST/Type.h:4199-4201
+ /// Return this attributed type's modified type with no qualifiers attached
to
+ /// it.
+ QualType getUnqualifiedType() const;
----------------
`getUnqualifiedType` means something very specific in Clang, and this isn't it.
`getModifiedType` would be a better name.
================
Comment at: clang/include/clang/AST/TypeLoc.h:1086
+struct MacroQualifiedLocInfo {};
+
----------------
This struct is where the `ExpansionLoc` should live.
================
Comment at: clang/lib/AST/TypePrinter.cpp:107-108
+ // Set for preventing printing of macros with the same name.
+ llvm::StringSet<> PrintedMacros;
+
----------------
I think this is more expensive than we need and unnecessary... see below.
================
Comment at: clang/lib/AST/TypePrinter.cpp:982
+ // we trim any attributes and go directly to the original modified type.
+ printBefore(T->getUnqualifiedType(), OS);
+
----------------
Instead of recursing to the modified type here, and using a separate set to
prevent printing the same macro twice, how about something like:
```
// Step over MacroQualifiedTypes from the same macro to find the type ultimately
// qualified by the macro qualifier.
QualType Inner = T->getModifiedType();
while (auto *InnerMQT = dyn_cast<MacroQualifiedType>(Inner)) {
if (InnerMQT->getMacroIdentifier() != T->getMacroIdentifier())
break;
Inner = InnerMQT->getModifiedType();
}
printBefore(Inner, OS);
```
(and likewise in `printAfter`). And perhaps the above loop should be in
`MacroQualifiedType::getModifiedType()` rather than here.
================
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()));
----------------
leonardchan wrote:
> 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.
Right, sorry, mistake on my part. To deal with this, you need to call
`isAt{Start,End}OfMacroExpansion` while collecting `StartLocs` and `EndLocs`,
and stop once you hit a location where they return `false`. (That would mean
that you never step from the location of `AS1` out to the location of `THING`.)
================
Comment at: clang/lib/Sema/SemaType.cpp:7373
+ llvm::StringMap<llvm::SmallVector<const IdentifierInfo *, 2>> FoundMacros;
state.setParsedNoDeref(false);
----------------
(Unused, please remove.)
================
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();
}
----------------
leonardchan wrote:
> 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
> ```
Can you give an example of the failures? The approach you're using here isn't
quite right, as it will skip over a `typedef` that's wrapped in another type
sugar node (when `T` is not a typedef but desugars to one). Something like `&&
AT->getAs<TypedefType>() == T->getAs<TypedefType>()` (that is: if we'd be
stripping off a `typedef` sugar node to reach the `AttributedType`, then stop)
should work, but maybe there's a better way to express that.
================
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());
+ }
----------------
leonardchan wrote:
> 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.
I think the ideal representation for this case would be
```
MacroQualifiedType (ATTRS)
`- AttributedType attr1
`- AttributedType attr2
`- int
```
... but I can see that'll be a pain to produce. Let's go with your current
representation for now and look into improving it as a follow-up change.
(This'll give weird behavior in some cases: removing the outer `AttributedType`
will still lead to a type that pretty-prints as `ATTRS int`, whereas it should
pretty-print as `__attribute__((attr2)) int` because `attr1` is not applied to
the type.)
================
Comment at: clang/lib/Sema/TreeTransform.h:6184
+
+ TLB.push<MacroQualifiedTypeLoc>(Result);
+ return Result;
----------------
(Copy the expansion loc here.)
================
Comment at: clang/lib/Serialization/ASTReader.cpp:6181
+ case TYPE_MACRO_DEFINED: {
+ if (Record.size() != 3) {
----------------
The name of this enumerator is out of date; should be `TYPE_MACRO_QUALIFIED`
================
Comment at: clang/test/SemaObjC/gc-attributes.m:12-21
+ f0(&a2); // expected-warning{{passing '__weak A **' to parameter of type
'__strong A **' discards qualifiers}}
}
void f1(__weak A**); // expected-note{{passing argument to parameter here}}
void test_f1() {
A *a;
----------------
This is wrong: we need to print the macro qualifier on the right in cases like
this so that it attaches to the pointer. You can use
`TypePrinter::canPrefixQualifier` to determine whether to print the macro name
on the left or the right.
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
