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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added inline comments.


================
Comment at: clang/lib/Sema/TreeTransform.h:6872
+  const BTFTagAttributedType *oldType = TL.getTypePtr();
+  StringRef Tag = oldType->getTag();
+  QualType modifiedType = getDerived().TransformType(TLB, TL.getModifiedLoc());
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > yonghong-song wrote:
> > > erichkeane wrote:
> > > > 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.
> > > I will try to do some experiment and simplify this. Indeed this is C and non templates are involved.
> > > We still need this though, since there are other non-template tree-transforms.
> > 
> > Are we sure any of them can be hit for this new type? It'd be nice to keep this out of the template instantiation bits if possible.
> I think this may be the only unresolved conversation in the review.
> Are we sure any of them can be hit for this new type? It'd be nice to keep this out of the
> template instantiation bits if possible.

Actually I am not sure. But looks like we have to have the function implemented due to
'automatic code generation'. Let me take a look. If we don't need it since the attribute for C
only. The implementation can be just a unreachable failure or somehow I will see whether I can
tweak the auto code generation to avoid instantiating this function. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120296/new/

https://reviews.llvm.org/D120296