[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think we're getting there, but all these changes mean I have another round of comments. None of them should be major though.

I'm also wondering whether it wouldn't be good to add a couple (one for each mangling scheme?) unit tests for the new class, as a way to show the way the API is supposed to be used (without all of the Symtab stuff floating around).

In https://reviews.llvm.org/D50071#1187849, @sgraenitz wrote:

> I quickly realized, that it needs a bigger change to properly address the aforementioned issues. Here's the major changes. Some documentation is todo.
>
> **StringRef to reused buffer** addressed with a split into first parse then get buffer. It seems more natural that the buffer may be reused. Also we can ask for `const char *` or a `StringRef`, which avoids the conversion issue.
>
>   MC.ParseFunctionBaseName();
>   llvm::StringRef base_name = MC.GetBufferRef();
>


I like this.

> The fallback case always uses `ConstString` now and it may be a bit slower than before, but IMHO that's fine.

I agree it wouldn't be a disaster, but it also wasn't hard to fix things so this works correctly without C strings. So, I just went ahead and committed r338995, which should allow you to avoid messing with C strings in this class completely.



================
Comment at: include/lldb/Core/RichManglingInfo.h:59
+    case ItaniumPartialDemangler:
+      assert(m_IPD_buf && "Parse before accessing buffer");
+      return llvm::StringRef(m_IPD_buf, m_IPD_str_len);
----------------
Is this the right assertion? As far as I can tell, you initialize m_IPD_buf in the constructor unconditionally.


================
Comment at: source/Core/Mangled.cpp:322-323
+      context.ParseFullName();
+      const char *demangled_cstr = context.GetBufferAsCString();
+      m_demangled.SetCStringWithMangledCounterpart(demangled_cstr, m_mangled);
+      return true;
----------------
C string no longer needed here.


================
Comment at: source/Core/RichManglingInfo.cpp:126
+  case ItaniumPartialDemangler: {
+    auto multi_in_out = m_IPD_size;
+    auto buf = m_IPD.getFunctionBaseName(m_IPD_buf, &multi_in_out);
----------------
sgraenitz wrote:
> Not sure about `multi_in_out` as a name here. It's hard to find one that fits the purpose and stays below 30 chars.. IPD calls it `N`.
The name makes no difference to me. However, if you think it makes sense, you could try passing `getFunctionBaseName` as a member function pointer into the helper function, so that all of the buffer handling is completely hidden inside that function.


================
Comment at: source/Core/RichManglingInfo.cpp:47
+      ParseFullName();
+      log->Printf("demangled itanium: %s -> \"%s\"", mangled.GetCString(),
+                  GetBufferAsCString());
----------------
`LLDB_LOG(log, demangled itanium: {0} -> \"{1}\"", mangled, GetBuffer());`
shorter and avoids C strings


================
Comment at: source/Core/RichManglingInfo.cpp:72
+        get<CPlusPlusLanguage::MethodName>(m_cxx_method_parser)->GetBasename();
+    return base_name.front() == '~';
+  }
----------------
will `base_name` always be non-empty (e.g. if the plugin failed to parse the string?). `base_name.startswith('~')` might be more appropriate here.


================
Comment at: source/Symbol/Symtab.cpp:386-387
+  MC.ParseFunctionDeclContextName();
+  const char *decl_context_cstr = MC.GetBufferAsCString();
+  if (decl_context_cstr == nullptr || decl_context_cstr[0] == '\0') {
+    // This has to be a basename
----------------
`MC.GetBuffer().empty()`


https://reviews.llvm.org/D50071