aaron.ballman added a comment.
I think we're pretty close; it looks like there's still one unresolved
conversation about template instantiation needs and whether we can remove that
code or not.
================
Comment at: clang/include/clang/AST/TypeLoc.h:923
+ void initializeLocal(ASTContext &Context, SourceLocation loc) {}
+
+ QualType getInnerType() const { return getTypePtr()->getWrappedType(); }
----------------
yonghong-song wrote:
> yonghong-song wrote:
> > aaron.ballman wrote:
> > > yonghong-song wrote:
> > > > aaron.ballman wrote:
> > > > > Do we also need something like this?
> > > > > https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TypeLoc.h#L1187
> > > > I didn't see getLocalDataSize() in AttributedTypeLoc so it is not
> > > > needed for AttributedTypeLoc. BTFTagAttributedTypeLoc usage is very
> > > > similar to AttributedTypeLoc, so I think we are fine here.
> > > The main difference is that `AttributedLocInfo` has a member and
> > > `BTFTagAttributedLocInfo` is empty. This is why I think it's closer to an
> > > `AdjustedLocInfo` object which also is an empty struct.
> > I think adding getLocalDataSize() return 0 should be OK. But it looks like
> > we don't need to do it, at least for BTFTagAttributedTypeLoc. The following
> > are some details.
> >
> > The parent class of BTFTagAttributedTypeLoc is ConcreteTypeLoc, which
> > contains:
> >
> > unsigned getLocalDataSize() const {
> > unsigned size = sizeof(LocalData);
> > unsigned extraAlign = asDerived()->getExtraLocalDataAlignment();
> > size = llvm::alignTo(size, extraAlign);
> > size += asDerived()->getExtraLocalDataSize();
> > return size;
> > }
> >
> > unsigned getExtraLocalDataSize() const {
> > return 0;
> > }
> >
> > unsigned getExtraLocalDataAlignment() const {
> > return 1;
> > }
> >
> > Manually calculating getLocalDataSize() can get the same return value 0. So
> > I think it is okay not to define getLocalDataSize in
> > BTFTagAttributedTypeLoc.
> >
> > The AdjustedLocInfo seems having some inconsistency, at least based on
> > comments:
> >
> > struct AdjustedLocInfo {}; // Nothing.
> > unsigned getLocalDataSize() const {
> > // sizeof(AdjustedLocInfo) is 1, but we don't need its address to be
> > unique
> > // anyway. TypeLocBuilder can't handle data sizes of 1.
> > return 0; // No data.
> > }
> > Maybe previously AdjustedLocInfo size is 1 and the implementation of
> > getLocalDataSize() to set size to 0 explicitly in which case, the function
> > is needed inside AdjustedTypeLoc. It might be needed now since
> > AdjustedLocInfo size is 0.
> >
> For
> > Maybe previously AdjustedLocInfo size is 1 and the implementation of
> > getLocalDataSize() to set size to 0 explicitly in which case, the function
> > is needed inside AdjustedTypeLoc. It might be needed now since
> > AdjustedLocInfo size is 0.
> I mean "it might not be needed now since AdjustedLocInfo size is 0".
Ahh, I see why we get away with it then. Thank you!
================
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:
> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120296/new/
https://reviews.llvm.org/D120296
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
