https://github.com/labath commented:
This is a partial review. I ran out of steam when I reached the Lexer component. I really think that one should be a patch of its own because: - all of the lookaheads and everything are sufficiently complicated to deserve the proper attention - the implementation is really amenable to unit testing (all you need is a string as an input) I also noticed that this implementation casts a much wider web when it comes to searching for global variables (it uses `Target::FindGlobalVariables`, which searches the entire process, whereas the current implementation only uses variables from the [current compile unit](https://github.com/llvm/llvm-project/blob/cbe583b0bd8d46b4e5edda463e19e6a24c0817bc/lldb/source/Target/StackFrame.cpp#L476). I think it would be best to stick to the existing implementation for now (ideally by even reusing the `GetInScopeVariableList` function), and saving the discussion about the pros and cons of changing that to a separate patch. The reason for that is I can find at least two benefits to the current implementation (it's faster and it's guaranteed to return the same variable you would get by compiling the expression in the compiler). I think it'd be best if that was isolated from the more technical/mundane aspects of parsing an expression. https://github.com/llvm/llvm-project/pull/120971 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
