[Lldb-commits] [PATCH] D117383: [lldb] Expose std::pair children for unordered_map

Fri Apr 29 10:53:14 PDT 2022

jingham added a comment.
Herald added a project: All.

Sorry for the delay here.

This output is definitely nicer than the previous version, so this change is desirable.

If you really are switching on unordered_map vrs. unordered_set behavior,  can you check the backend ValueObject's type to determine whether you are dealing with a map or set?  It might be nice to make that a little more explicit.  I can't see anything in this file that indicates that this synthetic child provider is used for both classes, so having something conditional on it being a map is surprising.

================
Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp:128
+          // __cc is a field of __hash_value_type, which is a typedef.
+          if (name == "__cc")
+            m_element_type = key_value_type.GetTypedefedType();
----------------
kastiglione wrote:
> This block of changes is needed to support `unordered_set`, because this synthetic provider is shared by `unordered_set` and `unordered_map`.
> 
> For `unordered_map`, the element type is an internal hash node. For `unoredered_set`, it's the type `T` from `unordered_set<T>`. This part of the code needs to know whether there's a hash node here, and to step down through the `__cc` field.
> 
> I am not sure what the best way to do this is. I could do a string prefix check against `std::__hash_value_type`. I could do a smarter full string equality check, but could that possibly have false negatives if the wrong string type name is constructed. As it currently is, I assume if there's a one or more fields, and if the first one is named `__cc`, then it's assumed to be an internal hash node.
> 
> Suggestions welcome.
IIUC, this depends on whether the backend is of type unordered_map or unordered_set?  Can you just check the type of the backend?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117383