yonghong-song added a comment.
@aaron.ballman Thanks for the review! I will address all your comments and will
also add tests with `_Generic` and `__attribute__((overloadable))`.
================
Comment at: clang/include/clang/AST/Type.h:4797
+
+ BTFTagAttributedType(QualType canon, QualType wrapped,
+ BTFTypeTagAttr *attr)
----------------
aaron.ballman wrote:
> You should fix the formatting issue, but also, the parameter names don't
> match the usual naming conventions (please don't replace `attr` with `Attr`
> though; that's a type name, so go with something else like `BTFAttr`).
Will do. Sorry, I didn't spend effort on formatting itself as I know I will
need to make revisions. But indeed should do it regardless!
================
Comment at: clang/include/clang/AST/Type.h:4804
+ QualType getWrappedType() const { return WrappedType; }
+ BTFTypeTagAttr *getAttr() const { return Attr; }
+
----------------
aaron.ballman wrote:
> Should this be returning a `const BTFTypeTagAttr *` instead?
Yes, will use 'const BTFTypeTagAttr *` in the next revision.
================
Comment at: clang/include/clang/AST/TypeLoc.h:904-917
+struct BTFTagAttributedLocInfo {
+ const Attr *TypeAttr;
+};
+
+/// Type source information for an btf_tag attributed type.
+class BTFTagAttributedTypeLoc
+ : public ConcreteTypeLoc<UnqualTypeLoc, BTFTagAttributedTypeLoc,
----------------
aaron.ballman wrote:
> This seems a bit suspicious to me -- we are store the same attribute twice
> (once on the `BTFTagAttributedType` and once on its type loc). Do we need to
> do that, or can we have the type loc reach through the type pointer to get
> the attribute from there? (Alternatively, do we want to require callers to
> have access to an `BTFTagAttributedTypeLoc` in order to query the attribute,
> similar to how `AttributedType`/`AttributedTypeLoc` work?)
Good point. Will change
```
struct BTFTagAttributedLocInfo {
const Attr *TypeAttr;
};
```
to
```
struct BTFTagAttributedLocInfo {
};
```
since we can get the attribute from the `BTFTagAttributedType` itself.
================
Comment at: clang/include/clang/AST/TypeLoc.h:925
+
+ void initializeLocal(ASTContext &Context, SourceLocation loc) {
+ setAttr(nullptr);
----------------
aaron.ballman wrote:
> `loc` is unused? (and should have its identifier removed if this is on
> purpose)
loc is needed in the parameter for this function. For example, in
QualifiedTypeLoc, we have
```
/// Initializes the local data of this type source info block to
/// provide no information.
void initializeLocal(ASTContext &Context, SourceLocation Loc) {
// do nothing
}
```
================
Comment at: clang/include/clang/Serialization/ASTRecordWriter.h:126-127
+ /// Write an Attr object.
+ void writeAttr(Attr *attr) { AddAttr(attr); }
+
----------------
aaron.ballman wrote:
> This doesn't appear to be used? Or is this called automagically because of
> the changes in `TypeProperties.td`?
It is used because the changes in `TypeProperties.td`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120296/new/
https://reviews.llvm.org/D120296
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
