On Fri, 2023-11-03 at 22:37 +0100, Phil Sutter wrote: > On Fri, Nov 03, 2023 at 05:25:13PM +0100, Thomas Haller wrote: > [...] > > + /* The json() hooks of "symbol_expr_ops" and > > "variable_expr_ops" are > > + * known to be NULL, but for such expressions we never > > expect to call > > + * expr_print_json(). > > + * > > + * All other expr_ops must have a json() hook. > > + * > > + * Unconditionally access the hook (and segfault in case > > of a bug). */ > > + return expr_ops(expr)->json(expr, octx); > > This does not make sense to me. You're deliberately dropping any > error > handling Error handling for what is clearly a bug. Don't try to handle bugs. Avoid bugs and fix them. > and accept a segfault because "it should never happen"? All it > takes is someone to add a new expression type and forget about the > JSON > API. There will be a unit test guarding against that, once the unit test basics are done. Also, if you "forget" to implement the JSON hook and test it (manually) only a single time, then you'll notice right away. > > If you absolutely have to remove that fallback code, at least add a > BUG() explaining the situation. The sysadmin looking at the segfault > report in syslog won't see your comment above. I am in favor of adding assertions all over the place. The project doesn't use enough asserions for my taste. In this case, it seems hard to mess up the condition, and you get a very clear signal when you do (segfault). That makes the assert()/BUG() kinda unecessary. But sure. Assert()/BUG() works too... Thomas
