[PATCH] D158296: [clang] Diagnose overly complex Record in __builtin_dump_struct

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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();
----------------
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++.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158296/new/

https://reviews.llvm.org/D158296