erichkeane added a comment.
I don't have enough knowledge about how this works to do a better review, but I
have a couple of suggestions.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:749
bool IsExtended) {
- Descriptor *D = P.createDescriptor(Src, Ty, IsConst, Src.is<const Expr *>());
+ const Descriptor::MetadataSize MDSize{sizeof(InlineDescriptor)};
+ Descriptor *D =
----------------
aaron.ballman wrote:
> Double checking that `Src.is<const Expr *>()` is correct here. That's being
> passed as the `IsTemporary` argument. I presume the logic is that if the decl
> type is an expression then it has to be a temporary, but that's not entirely
> true. Consider C where a compound literal is *not* a temporary but actually
> creates an lvalue: `(int){12}`. Will that be an issue?
See suggested comment below in InterpBlock.
================
Comment at: clang/lib/AST/Interp/Descriptor.cpp:207
: Source(D), ElemSize(primSize(Type)), Size(ElemSize * NumElems),
- AllocSize(align(Size) + sizeof(InitMap *)), IsConst(IsConst),
- IsMutable(IsMutable), IsTemporary(IsTemporary), IsArray(true),
- CtorFn(getCtorArrayPrim(Type)), DtorFn(getDtorArrayPrim(Type)),
- MoveFn(getMoveArrayPrim(Type)) {
+ MDSize(MD.NumBytes), AllocSize(align(Size) + sizeof(InitMap *) + MDSize),
+ IsConst(IsConst), IsMutable(IsMutable), IsTemporary(IsTemporary),
----------------
Theres enough math on stuff like AllocSize/etc that we should probably:
1- do SOMETHING to make sure we're not overflowing
2- Have assertions to make sure we got the sizes looking reasonable.
Additionally, is there any ability to refactor out the common subset of
initializers into a constructor and delegate to them instead of all these
really fragile looking changes?
================
Comment at: clang/lib/AST/Interp/Descriptor.h:74
+ };
+ static constexpr MetadataSize NoMetadata = MetadataSize{0};
+
----------------
add a line:
`static constexpr MetadataSize InlineDescriptor =
MetadataSize{sizeof(InlineDescriptor)};`
and you can use this instead of depending on 'sizeof' all over the place.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135750/new/
https://reviews.llvm.org/D135750
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
