This is an archive of the discontinued LLVM Phabricator instance.

Fix handling for the clang name mangling extension for block invocations
ClosedPublic

Authored by shafik on Nov 1 2019, 2:00 PM.

Diff Detail

Event Timeline

shafik created this revision.Nov 1 2019, 2:00 PM
jingham added inline comments.Nov 1 2019, 2:35 PM
lldb/source/Core/Mangled.cpp
99

StringRef has a startswith. That might be easier to read.

aprantl added inline comments.Nov 1 2019, 2:48 PM
lldb/include/lldb/Core/Mangled.h
264

Doxygen comment?

lldb/source/Core/Mangled.cpp
99

Agreed. There should be no performance penalty for using startswith over this in an optimized build.

102

Is ___Z really only used for blocks or is it used for other clang extensions, too?

104

Just FYI, this will create a merge conflict with swift-lldb. You might want to think about how to resolve it ahead of committing.

106

You don't need to fix this all at once, but I think it would be even better if this function did something like

for each language plugin { 
  if (mangling_scheme = plugin.isMangledName(...)
    ...
}

I.e., the plugins should be the ones that know about the mangling details.

310

If it's equivalent I'd find
ManglingScheme mangling_scheme = GetManglingScheme(m_mangled.GetStringRef());
easier to read.

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
104

That's a good idea anyway :-)

shafik updated this revision to Diff 227786.Nov 4 2019, 2:52 PM
shafik marked 12 inline comments as done.

Updating based on comments

  • Adding documentation
  • using startswith(...)
  • Updating initialization to use = instead of {}
shafik added inline comments.Nov 5 2019, 10:00 AM
lldb/source/Core/Mangled.cpp
99

Great catch!

102

This is solely for block invocations.

104

It does not look too bad.

106

There is an old comment akin to that in IsCPPMangledName. I did not want to stray too far off into refactoring in this PR.

aprantl added inline comments.Nov 5 2019, 11:20 AM
lldb/source/Core/Mangled.cpp
36

I was going to say, the function name doesn't make sense any more, but really we probably don't need this function at all, right?

106

I think IsCPPMangledName should be called IsMangledName and every language plugin should define one. The fact that you forgot to make the same bugfix in CPlusPlusLanguage::IsCPPMangledName should be motivation enough to not implement the same functionality in two places :-)

aprantl accepted this revision.Nov 5 2019, 1:54 PM

Let's do the refactoring in a follow-up commit.

This revision is now accepted and ready to land.Nov 5 2019, 1:54 PM
This revision was automatically updated to reflect the committed changes.
shafik marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2019, 2:21 PM