This is an archive of the discontinued LLVM Phabricator instance.

implement gdb-set output-radix
ClosedPublic

Authored by ki.stfu on Mar 18 2015, 5:21 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

ChuckR updated this revision to Diff 22232.Mar 18 2015, 5:21 PM
ChuckR retitled this revision from to implement gdb-set output-radix.
ChuckR updated this object.
ChuckR edited the test plan for this revision. (Show Details)
ChuckR added a reviewer: abidh.
ChuckR added a subscriber: Unknown Object (MLST).
abidh accepted this revision.Mar 19 2015, 2:30 AM
abidh edited edge metadata.

Looks ok apart from one change that seems like a different issue. So please remove that part. If will be good to add a test case too.

tools/lldb-mi/MICmdCmdVar.cpp
1064 ↗(On Diff #22232)

This change does not seem related to the rest of the patch. It will be better to submit it separately.

This revision is now accepted and ready to land.Mar 19 2015, 2:30 AM
ki.stfu requested changes to this revision.Mar 19 2015, 3:40 AM
ki.stfu added a reviewer: ki.stfu.
ki.stfu added a subscriber: ki.stfu.
ki.stfu added inline comments.
tools/lldb-mi/MICmdCmdVar.cpp
1060 ↗(On Diff #22232)

} newline
else newline
{

1064 ↗(On Diff #22232)

agree. please create separate review for it.

This revision now requires changes to proceed.Mar 19 2015, 3:40 AM
ChuckR updated this revision to Diff 22274.Mar 19 2015, 10:26 AM
ChuckR edited edge metadata.

removed extraneous fix. Can somebody please check this in on my behalf?

I agree with @abidh, that we should add tests for this. Could you do it?

ki.stfu requested changes to this revision.Mar 19 2015, 10:34 AM
ki.stfu edited edge metadata.
This revision now requires changes to proceed.Mar 19 2015, 10:34 AM

I agree with @abidh, that we should add tests for this. Could you do it?

Is there any documentation/guidance on adding tests? Or lldb/lldb-mi's testing infrastructure in general?

I agree with @abidh, that we should add tests for this. Could you do it?

Is there any documentation/guidance on adding tests? Or lldb/lldb-mi's testing infrastructure in general?

Please look in the test/tools/lldb-mi and you will see already present tests.

I agree with @abidh, that we should add tests for this. Could you do it?

Is there any documentation/guidance on adding tests? Or lldb/lldb-mi's testing infrastructure in general?

I'll add tests for -gdb-set/-gdb-show soon (it still on review D8566). So please add your tests into MiGdbSetShowTestCase like I did.

ChuckR updated this revision to Diff 23194.Apr 2 2015, 6:36 PM
ChuckR edited edge metadata.

added test

ChuckR added a comment.Apr 2 2015, 6:37 PM

All lld-mi tests passing.

ki.stfu requested changes to this revision.Apr 3 2015, 1:32 AM
ki.stfu edited edge metadata.

Looks good apart the following: move your test to MiGdbSetShowTestCase, please.

test/tools/lldb-mi/variable/TestMiVar.py
192 ↗(On Diff #23194)

Move this test to test/tools/lldb-mi/TestMiGdbSetShow.py

This revision now requires changes to proceed.Apr 3 2015, 1:32 AM
ChuckR updated this revision to Diff 23233.Apr 3 2015, 12:25 PM
ChuckR edited edge metadata.

moved test

ki.stfu accepted this revision.Apr 3 2015, 2:26 PM
ki.stfu edited edge metadata.
This revision is now accepted and ready to land.Apr 3 2015, 2:26 PM
ChuckR added a comment.Apr 6 2015, 3:03 PM

Can somebody check this in for me now that it has been approved?

Can somebody check this in for me now that it has been approved?

Yes, I can do that.

ki.stfu updated this object.Apr 7 2015, 2:43 AM
ki.stfu edited edge metadata.
ki.stfu commandeered this revision.Apr 7 2015, 2:46 AM
ki.stfu edited reviewers, added: ChuckR; removed: ki.stfu.
ki.stfu updated this revision to Diff 23315.Apr 7 2015, 2:48 AM

Rebase this against ToT

This revision was automatically updated to reflect the committed changes.