[lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[kastiglione]

Summary

Follow up to D117383, fixing the assumption that libc++ always uses __1 as
its inline namespace name.

[rupprecht]

LGTM -- I patched this in to verify it fixes the test failure we saw.

However, and not something that needs to happen in this patch (you could though), but the test actually isn't failing anymore even without this patch, because D129386 changed the naming convention from __cc to __cc_ I can get the test to correctly fail without this patch by changing the test assertion to:

must_not_contain__cc = r'(?s)^(?!.*\b__cc_ = )'

I'm not sure if there's a less fragile way to write that test. You could assert the entire string instead of looking for sub-fields, perhaps. Maybe that's hard to do for an unordered collection though.

[rupprecht]

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp

76

This might be overly generic: every libc++ std inline namespace I've seen is __[a-zA-Z0-9]+::, e.g. we should handle std::__foo::unordered_map but not std::foo::unordered_map. I don't know if we've codified that convention anywhere.

For comparison, see https://github.com/llvm/llvm-project/blob/839b436c93604e042f74050cf2adadd75f30e898/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp#L580

[kastiglione]

Thanks for the report about the __cc, I can try to come up with something less fragile.

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp

76

Happy to require the leading __.

[kastiglione]

Expect inline namespace to be __[[:alnum:]]+::

[kastiglione]

I updated the test in D133395

[rupprecht]

Thanks again! Still looks good.

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp

70

nit: this consumes just the inline namespace, so I think consumeInlineNamespace might be a better name. I don't feel that strongly though so I'll leave it up to you.

[JDevlieghere]

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp

70

Rather than modifying the passed-in string, would it make sense to return a llvm::StringRef?

[kastiglione]

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp

70

@JDevlieghere My answer is twofold: It follows the "consume" APIs on StringRef. And I prefer to avoid find expressions where you assign to a var being used on the right side:

someVar = someVar.method(...)

If you'd like me to change it, I will.

70

@rupprecht I agree, consumeInlineNamespace it is.

[kastiglione]

Rename to consumeInlineNamespace

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp

if (valobj_sp)
     Update();
 }
 
 size_t lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
     CalculateNumChildren() {
   return m_num_elements;
 }
 
 static bool isStdTemplate(ConstString type_name, llvm::StringRef type) {
   llvm::StringRef name = type_name.GetStringRef();
-  // The type name may or may not be prefixed with `std::` or `std::__1::`.
-  if (name.consume_front("std::"))
-    name.consume_front("__1::");
+  // The type name may be prefixed with `std::<inline-namespace>::`.
+  if (name.consume_front("std::")) {
+    // Delete past the inline namespace: [a-zA-Z0-9_]+::
+    auto ident_end =
+        name.find_if_not([](char c) { return std::isalnum(c) || c == '_'; });
+    if (ident_end != llvm::StringRef::npos && ident_end >= 1) {
+      auto temp = name.drop_front(ident_end);
+      if (temp.consume_front("::"))
+        name = temp;
+    }
+  }
   return name.consume_front(type) && name.startswith("<");
 }
 
 static bool isUnorderedMap(ConstString type_name) {
   return isStdTemplate(type_name, "unordered_map") ||
          isStdTemplate(type_name, "unordered_multimap");
 }