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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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());
----------------
yonghong-song wrote:
> 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. 
If we need it, we need it. But the more we can remove (like replacing the implementation with an unreachable), the better, so thank you for looking into it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120296