yonghong-song added a comment.
In D111199#3078184 <https://reviews.llvm.org/D111199#3078184>, @aaron.ballman
wrote:
> I reviewed this a bit, but I think it might be heading in a slightly wrong
> direction. I think you should be using an `AttributedType` but I don't think
> we need to add `AttributedBTFType` as a subclass to do this. An
> `AttributedType` holds the type kind information in it, but you should be
> able to access an `AttributedTypeLoc` object to get to the semantic attribute
> to access the string argument to the attribute. Generally, you'd call
> `getTypeSourceInfo()` on the declaration to get the type source info for its
> type, you can call `getTypeLoc()` on that object to get a generic `TypeLoc`
> object, and then call `getAs<AttributedTypeLoc>()` on that to turn it into an
> `AttributedTypeLoc` and from there you can call `getAttr()` to get the `Attr
> *` for the semantic attribute.
Thanks for detailed explanation. This is very useful. I will look into this. I
agree with your approach and I myself also have concern with
`AttributedBTFType`. I will just use `AttributedType` going forward.
================
Comment at: clang/include/clang/AST/PropertiesBase.td:134
def ExprRef : SubclassPropertyType<"Expr", StmtRef>;
+def String : PropertyType<"std::string">;
def TemplateArgument : PropertyType;
----------------
aaron.ballman wrote:
> Is `std::string` really the correct type to be using for AST serialization?
> Can we use `StringRef` here as well?
I think StringRef may be possible. I will investigate.
================
Comment at: clang/include/clang/AST/TypeLoc.h:896
+ : public InheritingConcreteTypeLoc<AttributedTypeLoc, AttributedBTFTypeLoc,
+ AttributedBTFType> {};
+
----------------
aaron.ballman wrote:
> Is this actually needed? I would have assumed the `AttributedTypeLoc` would
> suffice.
Honestly I am not 100%. But since we will AttributedType and this should not be
an issue any more.
================
Comment at: clang/include/clang/Basic/Attr.td:1850
+ let Args = [StringArgument<"BTFTypeTag">];
+ let Documentation = [Undocumented];
+ let LangOpts = [COnly];
----------------
aaron.ballman wrote:
> No new, undocumented attributes please.
Will do in the next revision.
================
Comment at: clang/include/clang/Serialization/ASTRecordWriter.h:286-287
+ void writeString(StringRef Str) { return AddString(Str); }
+
/// Emit a path.
----------------
aaron.ballman wrote:
> This appears to be unused, can be removed?
Not 100% sure whether this will be used during serialization/deserialization. I
will double check.
================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1152
+ StringRef BTFTypeTag = AT->getBTFTypeTag();
+ if (BTFTypeTag[0]) {
+ llvm::Metadata *Ops[2] = {
----------------
aaron.ballman wrote:
> Should this be checking `!BTFTypeTag.empty()` instead?
Yes, that is true. This probably from old previous revision leftover which I am
using a char array and the first '\0' indicating no attribute.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111199/new/
https://reviews.llvm.org/D111199
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
