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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
