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

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

Thanks or the quick reviews! Follow-ups inline.



================
Comment at: include/lldb/Symbol/Symtab.h:58
+  /// dependency. Keep a void* here instead and cast it on-demand on the cpp.
+  void *m_legacy_parser = nullptr;
+
----------------
zturner wrote:
> sgraenitz wrote:
> > This is the hackiest point I guess.
> We have `llvm::Any`.  Perhaps you want to use that here instead of `void*`?
Thanks. I will check that.


================
Comment at: include/lldb/Symbol/Symtab.h:81-83
+  // No move
+  RichManglingInfo(RichManglingInfo &&) = delete;
+  RichManglingInfo &operator=(RichManglingInfo &&) = delete;
----------------
labath wrote:
> This is implied by the deleted copy operations.
Which are implicitly deleted too, due to the existence of the destructor right? Does LLVM/LLDB have some kind of convention for it? I like to be explicit on ctors&assignment ("rule of 5"), because it aids error messages, but I would be fine with following the existing convention here.


================
Comment at: source/Symbol/Symtab.cpp:271-289
+const char *RichManglingInfo::getFunctionBaseName() const {
+  switch (m_provider) {
+  case ItaniumPartialDemangler:
+    m_IPD_buf = m_IPD->getFunctionBaseName(m_IPD_buf, &m_IPD_size);
+    return m_IPD_buf;
+  case PluginCxxLanguage:
+    return get<CPlusPlusLanguage::MethodName>()->GetBasename().data();
----------------
labath wrote:
> Could these return StringRef instead of C strings?
Yes. So far it's simply the closest superset of the two interfaces, but I will try using `StringRef` where possible.


================
Comment at: source/Symbol/Symtab.cpp:295
+namespace {
+bool lldb_skip_name(const char *mangled_cstr, Mangled::ManglingScheme scheme) {
+  switch (scheme) {
----------------
labath wrote:
> sgraenitz wrote:
> > This uses a raw C-string instead of `llvm::StringRef` in order to achieve `O(1)` runtime.
> If you changed the caller to use StringRef too (it seems possible at a first glance) then this would still be O(1)
Right, thanks there's a `ConstString::GetStringRef()`. Perfect.


https://reviews.llvm.org/D49990