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

[Erich Keane via Phabricator via cfe-commits]

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