rsmith 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(); ---------------- 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. 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
