This revision was automatically updated to reflect the committed changes.
Closed by commit rL345693: [lldb] Introduce StackFrameRecognizer [take 3]
(authored by kuba.brecka, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D44603?vs=171848=171860#toc
Repository:
rL LLVM
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB345686: [lldb] Introduce StackFrameRecognizer [take 2]
(authored by kuba.brecka, committed by ).
Herald added subscribers: teemperor, abidh.
Changed prior to commit:
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345678: [lldb] Introduce StackFrameRecognizer (authored by
kuba.brecka, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.
LGTM. Thanks for working on this!
https://reviews.llvm.org/D44603
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
kubamracek added a comment.
Ping.
https://reviews.llvm.org/D44603
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
kubamracek updated this revision to Diff 170809.
kubamracek added a comment.
Adding class comments, adding IDs to recognizers.
https://reviews.llvm.org/D44603
Files:
include/lldb/API/SBVariablesOptions.h
include/lldb/Interpreter/OptionGroupVariable.h
jingham added a comment.
This is good. The addition of the "info" command will be helpful for people
trying to debug their recognizers. It's okay to add multiple -s and -n's later
- though the fact that you don't allow "apply to all frames" may make us want
the ability to provide more than
aprantl added inline comments.
Comment at: include/lldb/Target/StackFrameRecognizer.h:25
+
+class RecognizedStackFrame
+: std::enable_shared_from_this {
Would you mind adding doxygen comments to each of the new classes to explain
what they are good for?
kubamracek updated this revision to Diff 170724.
kubamracek added a comment.
Updating patch, addressing most comments. Changed '-m' to '-s'. Added 'frame
recognizer info' subcommand. Improved wording in documentation.
I didn't add the possibility to specify multiple '-s' and '-n' args. Do you
jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.
This is good. I had a few inline comments, mostly about the command syntax. I
think you should switch "-m" to "-s" since that's what we use in the other
similar places.
For the
kubamracek added a comment.
friendly ping :)
https://reviews.llvm.org/D44603
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
kubamracek updated this revision to Diff 152017.
kubamracek added a comment.
Fixing up test (it was using the "-t" flag).
Removing a leftover printf().
https://reviews.llvm.org/D44603
Files:
include/lldb/API/SBVariablesOptions.h
include/lldb/Interpreter/OptionGroupVariable.h
kubamracek updated this revision to Diff 152004.
kubamracek added a comment.
Switched on showing recognized args by default in "frame variable". Switched
"-t" to mean "omit" instead of "include".
Added documentation and an example into "help frame recognizer add" and into
python-reference.html.
jingham added a comment.
This code looks fine to me. The docs are a little thin.
You don't say how to make a recognizer anywhere except in the test example.
There should probably be an example in the long help for
CommandObjectFrameRecognizerAdd. Look for instance at the help for
kubamracek updated this revision to Diff 151991.
kubamracek added a comment.
Rebasing to current master.
https://reviews.llvm.org/D44603
Files:
include/lldb/API/SBVariablesOptions.h
include/lldb/Interpreter/OptionGroupVariable.h
include/lldb/Interpreter/ScriptInterpreter.h
jingham added a comment.
I think ultimately we should make this presentation of variables as symmetrical
with locals as we can or it's going to confuse users.
I don't think it is necessary for the first version. The first uses, I would
imagine, are to make available some variables in
kubamracek added a comment.
In https://reviews.llvm.org/D44603#1133149, @jingham wrote:
> This is going as I imagined it should, looks great!
> We probably want to turn this on by default for "frame var" or no one will
> ever discover it. The IDE folks can decide on their own what to do from
jingham added a comment.
This is going as I imagined it should, looks great!
We probably want to turn this on by default for "frame var" or no one will ever
discover it. The IDE folks can decide on their own what to do from the SB
API's. Since you are doing module filtering and this only
kubamracek added inline comments.
Comment at: source/Commands/CommandObjectFrame.cpp:833
+bool CommandObjectFrameRecognizerAdd::DoExecute(Args ,
CommandReturnObject ) {
+ if (m_options.m_class_name.empty()) {
+result.AppendErrorWithFormat("%s needs a Python class name.\n",
kubamracek added a comment.
@jingham, @jasonmolenda, I haven't addresses the comments yet, but I'll do that
in the next round. I'd seeking approval of the high-level approach first. The
patch is getting a bit large, but the added test (and the Python implementation
of a recognizer) should
kubamracek updated this revision to Diff 150891.
kubamracek edited the summary of this revision.
kubamracek added a comment.
Updating StackFrameRecognizer with a Python plugin implementation and a
testcase using a Python recognizer. Also removing the Darwin-specific parts.
jingham added a comment.
This seems like a good place to start. Certainly producing synthetic function
arguments is one of the fixed things these recognizers can do, and we can build
on that as we go. The inputs are all types that we know how to bind to Python,
so it will be easy to make
kubamracek created this revision.
kubamracek added reviewers: jingham, jasonmolenda.
Herald added a subscriber: mgorny.
This is following on the discussion and goals of
https://reviews.llvm.org/D43886. The patch introduces a concept of "frame
recognizer" and "recognized frame". This should be
23 matches
Mail list logo
