labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp:45
if (name_type_mask & eFunctionNameTypeFull) {
- dies.push_back(die);
- return;
+ if (!die.IsMethod() || die.GetMangledName(false) == name) {
+ dies.push_back(die);
----------------
jarin wrote:
> jarin wrote:
> > labath wrote:
> > > I don't believe the `IsMethod` check is really needed here -- the mangled
> > > name check should handle everything.
> > >
> > > In fact, looking at the implementation of
> > > `DWARFDebugInfoEntry::GetMangledName`, I don't think you even need the
> > > extra `substitute_name_allowed=false` part. The default value should do
> > > exactly what we need. If the DIE has a linkage (mangled) name it will
> > > return it and we will use that for comparison. For an `extern "C"`
> > > function it will return the regular name, and we will compare that
> > > instead (this check is somewhat redundant because if the name doesn't
> > > match, the function should not be in the index in the first place, but I
> > > don't think it hurts to check either).
> > >
> > > Am I missing something?
> > I want to avoid returning methods to [[
> > https://github.com/llvm/llvm-project/blob/808142876c10b52e7ee57cdc6dcf0acc5c97c1b7/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp#L1258
> > | ClangExpressionDeclMap::LookupFunction ]], so that
> > ClangExpressionDeclMap::LookupFunction does not do the expensive [[
> > https://github.com/llvm/llvm-project/blob/808142876c10b52e7ee57cdc6dcf0acc5c97c1b7/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp#L1298
> > | DeclContext parsing ]] just to find out the function [[
> > https://github.com/llvm/llvm-project/blob/808142876c10b52e7ee57cdc6dcf0acc5c97c1b7/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp#L1304
> > | is a method ]] and must be thrown away.
> >
> > The motivation is pretty much the same as for
> > https://reviews.llvm.org/D70846.
> >
> > Does it make sense?
> I think I see what you are saying - in the test case below, FULL should be
> consistent with FULL-INDEXED. Let me remove the IsMethod check and merge the
> FULL and FULL-INDEXED test cases below.
Yes, that's exactly what I meant. :)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73191/new/
https://reviews.llvm.org/D73191
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
