bulbazord added a comment. In D129682#3651175 <https://reviews.llvm.org/D129682#3651175>, @labath wrote:
> Well.. first of let me say that the performance gain is impressive. I > wouldn't have expected to gain that much with a relatively simple change. Thank you! I am also very surprised, it turned out to be a worthwhile time investment. > - Do we really need the `GetQualifiedNameWithParams` function? My impression > was that we've been moving towards ignoring function arguments for name > matching, and `CPlusPlusLanguage::DemangledNameContainsPath` does not look > like it is making use of that. Note that I don't think that switching to > `GetQualifiedName` is the right approach either. I think we should use the > mangled name (`GetMangledName`) first, and only fall back to > `GetQualifiedName` if that is unavailable (you can look up how it works > during SymbolContext construction, but I believe it is roughly like that. > (Incidentally, that will bring in the function arguments through the back > door for some/most functions, but that isn't my motivation.) My reasoning for writing `GetQualifiedNameWithParams` instead of just using `GetQualifiedName` is because of matching overloaded names. It's valid for a user to set a breakpoint like `b a_function(int)` to disambiguate any other functions named `a_function`. Without the parameter list, we would be setting a breakpoint on every `a_function` which is not correct if you specify the argument list. I originally used `GetQualifiedName` but TestBreakOnOverload failed... That being said, I'm not sure how we would use a mangled name to perform the match. We're specifically matching on user input for breakpoints, so if a user inputs something like `StringView::begin`, how would we match that against mangled names? We would need some way to partially mangle `StringView::begin` and match that against the mangled name from the DIE, right? It would probably be faster to match against a mangled name though, so if there is a way to make that work that would be perfect. > - This patch is duplicating some of the logic inside > `Module::LookupInfo::Prune`. I get that we can't reuse it as-is (because we'd > need a `SymbolContext` object), but it seems like that function only cares > about the `GetFunctionName` portion of the object, so it seems like it should > be possible to refactor it such that one can reuse it from within this new > context (just pass in a name instead of a full SymbolContext). (I guess this > part could be a separate patch) Specifically you're suggesting a refactor of `Module::LookupInfo::Prune` so that we can deduplicate the logic right? I'd actually like to see if we can get rid of `Module::LookupInfo::Prune` altogether so we don't have to post-process search results but instead filter them as we find them. I hope I've accomplished at least some of that with this change, but getting rid of it entirely would require a few more changes in `Symtab` and in the other SymbolFile implementations (I think...). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129682/new/ https://reviews.llvm.org/D129682 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
