DavidSpickett wrote: So, the automatic checkers here are going to complain about formatting and style - ignore them for now. We'll review all that once the important stuff is done.
You should put this PR into "draft" mode, there is a button somewhere on the page (that only you can see). Just to make it clear to other reviewers. I have a few major things to mention, I wouldn't try to handle these all at once. Tackle them incrementally. I think you could extend the existing Dump functions by adding a new new name argument with a default value of nullptr. See https://en.cppreference.com/w/cpp/language/default_arguments and https://github.com/llvm/llvm-project/blob/9322a0c2f34a7c9f0d40e4f6c5d162d0fc06bd6c/lldb/include/lldb/Target/Target.h#L1193 For an example in tree. This means that all existing callers of all these dump functions can have the same result. Inside the functions you can say `if (name) print colours else print without`. By doing that, you do not need to make new versions of each dump function. Second, I'm looking at this printRed function thinking that this is a thing lldb must have done already elsewhere. That we'd have a utility function for. And we do. https://github.com/llvm/llvm-project/blob/cbbb545c4618969850d88bb008ab7f1c2918d5c3/lldb/source/Core/SourceManager.cpp#L265 That file does essentially what you're doing using it. You'd use the strings found [here](https://github.com/llvm/llvm-project/blob/cbbb545c4618969850d88bb008ab7f1c2918d5c3/lldb/include/lldb/Utility/AnsiTerminal.h#L83) instead of the ASNI_... So then you don't need to duplicate printRed everywhere. To be clear, I'm not criticizing you for not knowing this. Once you've been around a while you get a sense for these things and, well, it's also what reviewers are for :). Which brings me onto the fact that this feature will have to respect the `use-color` setting. The function mentioned above allows you to pass a bool to it to do that, you can look at other callers of it to see how they get that bool. To set this setting in lldb itself: ``` (lldb) process status <stuff with colours> (lldb) settings set use-color false (lldb) process status <stuff without colour> ``` And you can test it out manually. Next I think you'll benefit from writing some tests. Best thing to do is adapt existing ones. The tests for `target modules lookup -r -n` are in a shell test `lldb/test/Shell/Commands/command-target-modules-lookup.test`. So that's where I'd start. There are other shell tests that check colour (I'm British, color :) ) output, or lack of. If you search `lldb/test` for `use-color` you'll see them. You might have to turn colour off for the existing test, and add a new one that enables it and checks the ansi codes are there. To run just this test: ``` $ ./bin/llvm-lit ../llvm-project/lldb/test/Shell/Commands/command-target-modules-lookup.test -a ``` `-a` is not required but it means that if it fails, it will print out all the commands that it just ran so you can debug it. You showed some example output on Discourse and that's a decent starting point for the checks themselves. You had one where there was one match, many matches, etc. that's about all you'd need. And the reason I say to get some tests done is it'll preserve your sanity if you want to refactor the implementation. So my final thing is, I'm seeing `const char* name` passed around a lot and you having to strstr to find it. Even though the symbol lookup must have found at least 1 match already. I wonder if we can have an alternative interface that not only returns the indexes of symbols but the details of their matches. This might be too big of a change for this feature though, so let's deal with everything else first. As it's possible that the symbol table breaks after the first match in the string, so it wouldn't be able to return the full set of matches anyway. So it may mean adding an interface that does (in Python terms) `re.findall` instead of `re.search`. Anyway, that'll seem like a lot but take it one step at a time and feel free to update this as you go and/or ask more questions. Great work so far! https://github.com/llvm/llvm-project/pull/69422 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
