yronglin marked 4 inline comments as done. yronglin added a comment. Thank you for your review @aaron.ballman @rsmith , I will be happy to continue cook this patch once we reach a consensus.
================ 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(); ---------------- 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. 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
