[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[Dave Lee via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb66da73a05ab: [lldb] Dont assume name of libc++ inline 
namespace in LibCxxUnorderedMap (authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

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


Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -67,11 +67,23 @@
   return m_num_elements;
 }
 
+static void consumeInlineNamespace(llvm::StringRef ) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::
+  auto scratch = name;
+  if (scratch.consume_front("__") && std::isalnum(scratch[0])) {
+scratch = scratch.drop_while([](char c) { return std::isalnum(c); });
+if (scratch.consume_front("::")) {
+  // Successfully consumed a namespace.
+  name = scratch;
+}
+  }
+}
+
 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::`.
+  // The type name may be prefixed with `std::__::`.
   if (name.consume_front("std::"))
-name.consume_front("__1::");
+consumeInlineNamespace(name);
   return name.consume_front(type) && name.startswith("<");
 }
 


Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -67,11 +67,23 @@
   return m_num_elements;
 }
 
+static void consumeInlineNamespace(llvm::StringRef ) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::
+  auto scratch = name;
+  if (scratch.consume_front("__") && std::isalnum(scratch[0])) {
+scratch = scratch.drop_while([](char c) { return std::isalnum(c); });
+if (scratch.consume_front("::")) {
+  // Successfully consumed a namespace.
+  name = scratch;
+}
+  }
+}
+
 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::`.
+  // The type name may be prefixed with `std::__::`.
   if (name.consume_front("std::"))
-name.consume_front("__1::");
+consumeInlineNamespace(name);
   return name.consume_front(type) && name.startswith("<");
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[Dave Lee via Phabricator via lldb-commits]

kastiglione updated this revision to Diff 460778.
kastiglione added a comment.

Rename to consumeInlineNamespace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

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


Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -67,11 +67,23 @@
   return m_num_elements;
 }
 
+static void consumeInlineNamespace(llvm::StringRef ) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::
+  auto scratch = name;
+  if (scratch.consume_front("__") && std::isalnum(scratch[0])) {
+scratch = scratch.drop_while([](char c) { return std::isalnum(c); });
+if (scratch.consume_front("::")) {
+  // Successfully consumed a namespace.
+  name = scratch;
+}
+  }
+}
+
 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::`.
+  // The type name may be prefixed with `std::__::`.
   if (name.consume_front("std::"))
-name.consume_front("__1::");
+consumeInlineNamespace(name);
   return name.consume_front(type) && name.startswith("<");
 }
 


Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -67,11 +67,23 @@
   return m_num_elements;
 }
 
+static void consumeInlineNamespace(llvm::StringRef ) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::
+  auto scratch = name;
+  if (scratch.consume_front("__") && std::isalnum(scratch[0])) {
+scratch = scratch.drop_while([](char c) { return std::isalnum(c); });
+if (scratch.consume_front("::")) {
+  // Successfully consumed a namespace.
+  name = scratch;
+}
+  }
+}
+
 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::`.
+  // The type name may be prefixed with `std::__::`.
   if (name.consume_front("std::"))
-name.consume_front("__1::");
+consumeInlineNamespace(name);
   return name.consume_front(type) && name.startswith("<");
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp:70
 
+static void consumeNamespace(llvm::StringRef ) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::

JDevlieghere wrote:
> rupprecht wrote:
> > 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.
> Rather than modifying the passed-in string, would it make sense to return a 
> `llvm::StringRef`? 
@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.



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp:70
 
+static void consumeNamespace(llvm::StringRef ) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::

kastiglione wrote:
> JDevlieghere wrote:
> > rupprecht wrote:
> > > 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.
> > Rather than modifying the passed-in string, would it make sense to return a 
> > `llvm::StringRef`? 
> @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.
@rupprecht I agree, `consumeInlineNamespace` it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp:70
 
+static void consumeNamespace(llvm::StringRef ) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::

rupprecht wrote:
> 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.
Rather than modifying the passed-in string, would it make sense to return a 
`llvm::StringRef`? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht accepted this revision.
rupprecht added a comment.

Thanks again! Still looks good.




Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp:70
 
+static void consumeNamespace(llvm::StringRef ) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

I updated the test in D133395 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[Dave Lee via Phabricator via lldb-commits]

kastiglione updated this revision to Diff 458347.
kastiglione added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

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


Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -67,11 +67,23 @@
   return m_num_elements;
 }
 
+static void consumeNamespace(llvm::StringRef ) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::
+  auto scratch = name;
+  if (scratch.consume_front("__") && std::isalnum(scratch[0])) {
+scratch = scratch.drop_while([](char c) { return std::isalnum(c); });
+if (scratch.consume_front("::")) {
+  // Successfully consumed a namespace.
+  name = scratch;
+}
+  }
+}
+
 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::`.
+  // The type name may be prefixed with `std::__::`.
   if (name.consume_front("std::"))
-name.consume_front("__1::");
+consumeNamespace(name);
   return name.consume_front(type) && name.startswith("<");
 }
 


Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -67,11 +67,23 @@
   return m_num_elements;
 }
 
+static void consumeNamespace(llvm::StringRef ) {
+  // Delete past an inline namespace, if any: __[a-zA-Z0-9_]+::
+  auto scratch = name;
+  if (scratch.consume_front("__") && std::isalnum(scratch[0])) {
+scratch = scratch.drop_while([](char c) { return std::isalnum(c); });
+if (scratch.consume_front("::")) {
+  // Successfully consumed a namespace.
+  name = scratch;
+}
+  }
+}
+
 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::`.
+  // The type name may be prefixed with `std::__::`.
   if (name.consume_front("std::"))
-name.consume_front("__1::");
+consumeNamespace(name);
   return name.consume_front(type) && name.startswith("<");
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

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




Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp:76
+auto ident_end =
+name.find_if_not([](char c) { return std::isalnum(c) || c == '_'; });
+if (ident_end != llvm::StringRef::npos && ident_end >= 1) {

rupprecht wrote:
> 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
Happy to require the leading `__`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp:76
+auto ident_end =
+name.find_if_not([](char c) { return std::isalnum(c) || c == '_'; });
+if (ident_end != llvm::StringRef::npos && ident_end >= 1) {

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht accepted this revision.
rupprecht added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133259

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133259: [lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap

[Dave Lee via Phabricator via lldb-commits]

kastiglione created this revision.
kastiglione added a reviewer: rsmith.
Herald added a project: All.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133259

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


Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -69,9 +69,17 @@
 
 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`.
+  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("<");
 }
 


Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -69,9 +69,17 @@
 
 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`.
+  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("<");
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits