erichkeane added a comment.
I only looked at the 'new type' code, and it generally looks correct with the
exception of the comments below.
================
Comment at: clang/include/clang/AST/Type.h:4793
+
+ QualType ModifiedType;
+ QualType EquivalentType;
----------------
I suspect both of these aren't necessary. If the intent here is to just wrap a
single type, I think we can have only 1. Then the fields here are just the
Tag, and the WrappedType.
================
Comment at: clang/include/clang/Serialization/ASTRecordReader.h:131
+ StringRef readStringRef() { return readString(); }
+
----------------
This is a bug. readString returns a std::string temporary, which creating the
StringRef to, it now no longer exists.
================
Comment at: clang/lib/Sema/SemaType.cpp:194
+ std::pair<const BTFTagAttributedType*, const Attr*>;
+ SmallVector<BTFTagTypeAttrPair, 8> AttrsForBTFTagTypes;
+ bool AttrsForBTFTagTypesSorted = true;
----------------
This seems pretty absurdly expensive here... Should we instead have the
BTFTagAttributedType store its Attr here?
================
Comment at: clang/lib/Sema/TreeTransform.h:6872
+ const BTFTagAttributedType *oldType = TL.getTypePtr();
+ StringRef Tag = oldType->getTag();
+ QualType modifiedType = getDerived().TransformType(TLB, TL.getModifiedLoc());
----------------
Most of this tree-transform doesn't really do much, since this is a 'C' only
type, but otherwise we'd probably want to allow the tag itself to be dependent.
We still need this though, since there are other non-template tree-transforms.
You also might need to make this not contain a `StringRef` based on the
serialization issues above.
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
