kuilpd wrote: > I have several concerns with the way this PR implements the simple/full DIL > idea (BTW, I do like that you added "legacy" to the set of options). My > biggest concern is that this does not give users any control over the option > at all, unless they are LLVM programmers and can spin their own copy of LLDB > and program the change they want. Also, I am dismayed at the number of places > that have to be updated if we change the parameters for > GetValueForVariablePath/GetValueForVariableExpressionPath. (In addition, I'm > not sure that it's OK to change the parameters to functions in the SB API).
If you're talking about the users of API, then they can just provide the mode in an API call, like I did in the tests in this patch. The reason I made so many changes is that so this argument can be passed from anywhere (including API), and by default it comes in full mode, so that any existing calls that don't use it still work like before, like `frame var` command that I left unchanged. I'm not sure about changes in SB API as well, but since I added a default value for the argument so that it doesn't need to be specified, it seems to work just fine and all the existing tests pass. > I would MUCH rather make this a setting that users can control. Then we don't > have to touch all the places GetValueForVariablePath (or > GetValueForVariableExpressionPath) is called, we don't have to worry about > changing the SB API, and (as I said) users can easily change the setting if > they want different behavior than whatever default we set. If we're talking about end users of LLDB (and lldb-dap) in general, then yes, we still need to add specific LLDB settings to control specific behaviors, I think this should be done in separate patches. But if we have even 2 callers requesting different behavior from DIL (`frame var` wants full mode, `print` wants simple), how do we setup DIL to know where is the call coming from? With an argument we can have any number of callers, and we can separately introduce LLDB settings to control the behavior of each caller by end users. This patch is a fix for already varying demands between `frame var`, `print` and lldb-dap, and also the groundwork for adding settings. Even if we add only one setting option (for lldb-dap, for example), `frame var` will always want to use DIL in full mode regardless of that setting. > Could you add to the PR description the use-cases for this. I'm getting a bit > worried about the number of different ways things can be printed. So > motivating examples would be great @Michael137 I'm not quite sure what you mean by "different ways things can be printed"... which things are we talking about? The use case is that callers of DIL need to control if they want limited or full functionality, should I write `frame var` and DWIMPrint as examples? https://github.com/llvm/llvm-project/pull/178747 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
