[Lldb-commits] [lldb] [lldb] refactor highlighting function for image lookup command (PR #76112)

[David Spickett via lldb-commits]

================
@@ -1618,22 +1621,25 @@ static uint32_t LookupSymbolInModule(CommandInterpreter &interpreter,
     for (uint32_t i = 0; i < num_matches; ++i) {
       Symbol *symbol = symtab->SymbolAtIndex(match_indexes[i]);
       if (symbol) {
+        Stream::HighlightSettings pattern_info(
+            name, interpreter.GetDebugger().GetRegexMatchAnsiPrefix(),
+            interpreter.GetDebugger().GetRegexMatchAnsiSuffix());
         if (symbol->ValueIsAddress()) {
           DumpAddress(
               interpreter.GetExecutionContext().GetBestExecutionContextScope(),
               symbol->GetAddressRef(), verbose, all_ranges, strm,
-              use_color && name_is_regex ? name : nullptr);
+              use_color && name_is_regex
+                  ? std::optional<Stream::HighlightSettings>{pattern_info}
+                  : std::nullopt);
----------------
DavidSpickett wrote:

I'm thinking about how to simplify the logic here. While not having to build a struct every single time.

First, you could move the `Stream::HighlightSettings` construction outside the loop ("hoist" some people call that). This means if we match 1 million symbols, we only construct one highlight settings object.

If we know that `use_colour && name_is_regex` is false, we could set `name` in the `HighlightSettings` to `""`. Then when it's passed to `DumpAddress` it will see the pattern is empty and print normally and exit early.

That means you could have:
```
DumpAddress(
              interpreter.GetExecutionContext().GetBestExecutionContextScope(),
              symbol->GetAddressRef(), verbose, all_ranges, strm, pattern_info);
```
Always passing it shouldn't cost that much and removing the ternary removes a potential branch which balances that out a bit.

However, this makes the parameter being `optional` kinda pointless, and I think it should still be optional for every other caller who can just use `nullopt` and not pay the cost of making a `HighlightSettings`.

So in summary: I would move the `HighlightSettings` construction out of the loop. The rest of my idea I think is marginal and risks over complicating things.

https://github.com/llvm/llvm-project/pull/76112