This is an archive of the discontinued LLVM Phabricator instance.

Add low-frame/high-frame options to -stack-list-arguments (MI)
ClosedPublic

Authored by ki.stfu on Mar 12 2015, 12:37 AM.

Diff Detail

Event Timeline

ki.stfu updated this revision to Diff 21805.Mar 12 2015, 12:37 AM
ki.stfu retitled this revision from to Add low-frame/high-frame options to -stack-list-arguments (MI).
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added reviewers: abidh, clayborg.
ki.stfu added subscribers: abidh, clayborg, Unknown Object (MLST).
ki.stfu updated this revision to Diff 21808.Mar 12 2015, 1:16 AM

Add tests

abidh accepted this revision.Mar 12 2015, 3:47 AM
abidh edited edge metadata.

Looks good apart from a few comments. Please handle those before committing.

tools/lldb-mi/MICmdCmdStack.cpp
568

You have already checked the GetFound() in if. Why you need it again inside.

575

As I understand, you are trying to check in this else that either low or high is set but not both. Please put a comment as it is not obvious.

591–592

The description of these options says
"It is an error if low-frame is larger than the actual number of frames."

So that check should also be done here.

This revision is now accepted and ready to land.Mar 12 2015, 3:47 AM
ki.stfu planned changes to this revision.Mar 12 2015, 4:07 AM
ki.stfu updated this revision to Diff 21835.Mar 12 2015, 8:36 AM
ki.stfu edited edge metadata.

Fix remarks

This revision is now accepted and ready to land.Mar 12 2015, 8:36 AM
ki.stfu added inline comments.Mar 12 2015, 8:36 AM
tools/lldb-mi/MICmdCmdStack.cpp
568

stupid typo. thx!

ki.stfu closed this revision.Mar 12 2015, 8:38 AM
clayborg edited edge metadata.Mar 12 2015, 9:45 AM

Looks good