[Lldb-commits] [PATCH] D69738: Fix handling for the clang name mangling extension for block invocations

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.


================
Comment at: lldb/include/lldb/Core/Mangled.h:264
 
+  static Mangled::ManglingScheme GetManglingScheme(llvm::StringRef s);
+
----------------
Doxygen comment?


================
Comment at: lldb/source/Core/Mangled.cpp:99
+
+  if (s.size() >= 2 && (s[0] == '_' && s[1] == 'Z'))
+    return Mangled::eManglingSchemeItanium;
----------------
jingham wrote:
> StringRef has a startswith.  That might be easier to read.
Agreed. There should be no performance penalty for using startswith over this in an optimized build.


================
Comment at: lldb/source/Core/Mangled.cpp:102
+
+  // ___Z is a clang extension of block invocations
+  if (s.size() >= 4 && s[0] == '_' && s[1] == '_' && s[2] == '_' && s[3] == 'Z')
----------------
Is `___Z` really only used for blocks or is it used for other clang extensions, too?


================
Comment at: lldb/source/Core/Mangled.cpp:104
+  if (s.size() >= 4 && s[0] == '_' && s[1] == '_' && s[2] == '_' && s[3] == 'Z')
+    return Mangled::eManglingSchemeItanium;
+
----------------
Just FYI, this will create a merge conflict with swift-lldb. You might want to think about how to resolve it ahead of committing.


================
Comment at: lldb/source/Core/Mangled.cpp:106
+
+  return Mangled::eManglingSchemeNone;
+}
----------------
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.


================
Comment at: lldb/source/Core/Mangled.cpp:310
     const char *mangled_name = m_mangled.GetCString();
-    ManglingScheme mangling_scheme{cstring_mangling_scheme(mangled_name)};
+    ManglingScheme mangling_scheme{GetManglingScheme(m_mangled.GetStringRef())};
     if (mangling_scheme != eManglingSchemeNone &&
----------------
If it's equivalent I'd find 
`ManglingScheme mangling_scheme = GetManglingScheme(m_mangled.GetStringRef());`
easier to read.


================
Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h:104
 
-  static bool IsCPPMangledName(const char *name);
+  static bool IsCPPMangledName(llvm::StringRef name);
 
----------------
That's a good idea anyway :-)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69738/new/

https://reviews.llvm.org/D69738