[PATCH] D120296: [Attr] Fix a btf_type_tag AST generation bug

[Yonghong Song via Phabricator via cfe-commits]

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