dblaikie added inline comments.
================ Comment at: clang/lib/Sema/SemaChecking.cpp:732-733 + int RDKind = RD->isClass() ? 0 : (RD->isStruct() ? 1 : 2); + S.Diag(PtrArg->getBeginLoc(), diag::err_builtin_dump_struct_too_complex) + << RDKind << RD->getName(); + return ExprError(); ---------------- yronglin wrote: > rsmith wrote: > > aaron.ballman wrote: > > > rsmith wrote: > > > > aaron.ballman wrote: > > > > > This will correctly handle diagnosing a gigantic anonymous struct. > > > > Producing an error here seems likely to eventually cause problems in > > > > practice for some users: people are using `__builtin_dump_struct` in > > > > generic code for reflection purposes, not just for debugging, and this > > > > will cause us to start rejecting complex generic code. > > > > > > > > Instead of rejecting, can we produce a tree of `PseudoObjectExpr`s if > > > > we have too many steps to store in a single expression? > > > > Producing an error here seems likely to eventually cause problems in > > > > practice for some users: people are using __builtin_dump_struct in > > > > generic code for reflection purposes, not just for debugging, and this > > > > will cause us to start rejecting complex generic code. > > > > > > > > Instead of rejecting, can we produce a tree of PseudoObjectExprs if we > > > > have too many steps to store in a single expression? > > > > > > I think that requires wider discussion -- I don't think > > > `__builtin_dump_struct` is a reasonable interface we want to support for > > > reflection (in fact, I'd argue it's an explicit non-goal, the same as > > > reflection via `-ast-dump`). Compile-time reflection is something we're > > > likely to need to support more intentionally and I don't think we're > > > going to want to use this as an interface for it or have to maintain it > > > as a reflection tool long-term. As such, I think producing a tree of > > > `PseudoObjectExpr`s is overkill; you can quote me on this a few years > > > from now when we're laughing at its quaintness, but "16k fields of debug > > > output is enough for anyone" for a debugging interface. > > > > > > (That said, I think we should be considering what support we want to add > > > to the compiler for reflection in service of the work being done in WG21 > > > on the topic -- if `__builtin_dump_struct` is being used for reflection > > > in practice, it would be nice to give people a supported, more ergonomic > > > interface for it that we can use for a future version of C++.) > > The bug report https://github.com/llvm/llvm-project/issues/63169 was > > encountered by a user hitting the previous 256-element limit in practice > > when using `__builtin_dump_struct` for reflection. I don't think we can > > reasonably prevent that from happening, other than -- as you say -- > > encouraging WG21 to give us a real reflection design we can implement. > fixed. @rsmith what do you think we should do now? Support even larger values than uint16_t worth? (the tree `PseudoObjectExpr` suggestion you made?) If we did make the uint16_t limit stand - perhaps that'd apply pressure to folks who have been using this for RTTI to consider other options - it wouldn't break any existing code (that's not already broken in clang, at least - with the overflow stuff happening) but might put pressure on further growth of such techniques? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158296/new/ https://reviews.llvm.org/D158296 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
