kwk added a comment. In D77108#1952039 <https://reviews.llvm.org/D77108#1952039>, @labath wrote:
> 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. @labath okay, that makes sense. Sorry. 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
