labath added a comment. In D77108#1951997 <https://reviews.llvm.org/D77108#1951997>, @kwk wrote:
> In D77108#1951879 <https://reviews.llvm.org/D77108#1951879>, @labath wrote: > > > Most DW_OP cases check their stack, but it's quite possible that others > > were missed too. It might be a nice cleanup to make a function like > > (`getMinimalStackSize(op)`) and move this check up in front of the big > > switch. That could not handle all operators, as for some of them, the value > > is not statically known, but it would handle the vast majority of them. > > > @labath I somewhat like that the logic for each `op` is close to the `case` > statement. Creating the function that you suggested would separate this check > out somewhere else and you would have to look at two places. At least for > code review, the current solution is much clearer to me. I don't have a problem with the current patch (modulo the test tweak) -- it fixes a real problem and it follows the style of the surrounding code. However, DWARFExpression::Evaluate is gigantic (2600LOC), and nearly half of that is error handling. Tastes may vary, but I think that's a bigger readability problem than having to look at two places to understand an opcode. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77108/new/ https://reviews.llvm.org/D77108 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
