Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[lldb] Tab completion for process load/unload

Authored by MrHate on May 13 2020, 10:30 AM.


  1. Complete process load with the common disk file completion, so there is not test provided for it;
  2. Complete process unload with the tokens of valid loaded images.

Thanks for Raphael's help on the test for process unload.

Diff Detail

Event Timeline

MrHate created this revision.May 13 2020, 10:30 AM
teemperor requested changes to this revision.May 15 2020, 3:38 AM

I'm overall very happy with this patch. I have one change request though to prevent that we keep copy-pasting checks like if (m_exe_ctx.HasProcessScope()) around. See my inline comment for that. But beside that and the small typo I pointed out this is good to go.


Small detail: LLVM comments always have to end with a period '.' like a real sentence. Same with the sentence below.


I think we could prevent manual checks like this by checking the flags like eCommandRequiresProcess we set for the object (see the flags set in the constructor). If the command requires a process to be launched to do anything then I guess the same goes for the tab completion.

So I think the right way to do this is to never invoke HandleArgumentCompletion for a CommandObject that has eCommandRequiresProcess set if there current execution context doesn't have a process. I think this should be a separate patch (that doesn't need its own tests, but will be tested by the tests in here).

This revision now requires changes to proceed.May 15 2020, 3:38 AM
MrHate updated this revision to Diff 264480.May 17 2020, 2:31 AM

Removed local requirements checking. Added periods.

MrHate updated this revision to Diff 265914.May 24 2020, 1:24 AM
MrHate marked 2 inline comments as done.

Maybe we can skip the unified requirements checking now, coz it has got many things to deal with.
Added back the local checking here.

teemperor requested changes to this revision.Jul 6 2020, 2:51 AM

Can you also add a quick test for process load? Just copy the test_log_dir test but replace the dir completion with file completion (and you can just complete some dummy file, e.g. some empty you can create in the test directory).

Beside that this LGTM.


Btw, we can now do self.assertSuccess(err) (which will print a nice error on its own).

This revision now requires changes to proceed.Jul 6 2020, 2:51 AM
MrHate updated this revision to Diff 275742.Jul 6 2020, 9:42 AM
  1. added a test case for process load;
  2. replaced assertTrue(err.Success()...) with assertSuccess(err).
JDevlieghere added inline comments.Jul 7 2020, 10:06 AM

Spurious newline


In D83309 you wrote

if (!m_exe_ctx.HasTargetScope() || request.GetCursorIndex())

and CommandObjectProcessSignal::HandleArgumentCompletion in this file has

if (!m_exe_ctx.HasProcessScope() || request.GetCursorIndex() != 0)

Can we pick one and use (I like if (!m_exe_ctx.HasProcessScope() || request.GetCursorIndex())) and use that everywhere?


Range-based for:

for (lldb::addr_t token : process->GetImageTokens())
MrHate updated this revision to Diff 276301.Jul 7 2020, 7:04 PM
  1. replaced request.GetCursorIndex() != 0 with request.GetCursorIndex();
  2. renamed the loop variable at line 1005.
MrHate marked an inline comment as done.Jul 7 2020, 7:08 PM
MrHate added inline comments.

Maybe the loop variable name here led to misunderstanding. Cause I still need the index here, I cannot replace it with range-based for-loop.

JDevlieghere added inline comments.Jul 7 2020, 8:51 PM

You're right, I missed that. It might still be less code to just have a range-based for-loop and have a counter?

MrHate added inline comments.Jul 7 2020, 9:40 PM

With range-based for-loop I may code like this:

size_t token_idx = 0;
for (lldb::addr_t token : process->GetImageTokens()) {

The number of variable declaration lines is reduced by one, but there will be a counter increase line added. I think refactoring like this might not work much better. Do you have some suggestions?

This revision is now accepted and ready to land.Aug 20 2020, 1:16 PM
This revision was automatically updated to reflect the committed changes.