[Lldb-commits] [PATCH] D113760: [formatters] Draft diff for unordered_map libstdcpp formatter

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

When you do your tests, don't care too much about std::string yet, as it has its own issues, but instead create a non-standard size type like

  struct Foo {
    int a;
    int b;
    int c;
  };
  
  ...
   std::unordered_map<int, Foo> u;



================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:16-24
+        logger = lldb.formatters.Logger.Logger()
+        list_type = self.valobj.GetType().GetUnqualifiedType()
+        if list_type.IsReferenceType():
+            list_type = list_type.GetDereferencedType()
+        if list_type.GetNumberOfTemplateArguments() > 0:
+            data_type = list_type.GetTemplateArgumentType(0)
+        else:
----------------
you can simplify this


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:18
+        list_type = self.valobj.GetType().GetUnqualifiedType()
+        if list_type.IsReferenceType():
+            list_type = list_type.GetDereferencedType()
----------------
you are already passing stl_deref_flags when you declare this formatter in C++, so you shouldn't get a Reference type here


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:21
+        if list_type.GetNumberOfTemplateArguments() > 0:
+            data_type = list_type.GetTemplateArgumentType(0)
+        else:
----------------
GetTemplateArgumentType(0) will give you the key, and GetTemplateArgumentType(1) should give you the value type. The idea is that in this place you get both key_type and value_type, so that you don't need to figure them out later


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:44
+    def get_child_index(self, name):
+        logger = lldb.formatters.Logger.Logger()
+        try:
----------------
remove if not used


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:55
+            current = self.next     
+            while offset > 0:
+                current = current.GetChildMemberWithName('_M_nxt')
----------------
this looks weird. you are already doing a while loop in get_child_at_index that is taking you to the node you want, but you do again another loop. You have to use `current` instead. My suggestion is to remove this function and make everything happen in get_child_at_index with one single loop


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:71
+        try:
+            offset = self.count - index - 1
+            current = self.next     
----------------
why do you do this? that means that you are traversing backwards. Just move an 'index' number of times. The unordered map is unordered by definition


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:77
+            key = self.get_child_key(index)
+            return current.CreateChildAtOffset('[' + str(key.GetValue()) + ']', self.next.GetType().GetByteSize() + self.data_size, self.data_type)
+        
----------------
danilashtefan wrote:
> I assume that the cause of the first issue (different key value type) is located here. self.next.GetType().GetByteSize() + self.data_size does not work in this case. Key is read, however value - not. It means that we cannot always add self.data_size.
> 
> I tried to manually figure out on the unordered_map<char, int> case what can I add instead of self.data_size (which is 1 in this case), but got no success
do what i mentioned above regarding fetching the key and value types. I imagine that the internal allocator is using a pair<key, value>, which means that the memory layout will be:
  key
  value [with an offset of key size]


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:77
+            key = self.get_child_key(index)
+            return current.CreateChildAtOffset('[' + str(key.GetValue()) + ']', self.next.GetType().GetByteSize() + self.data_size, self.data_type)
+        
----------------
wallace wrote:
> danilashtefan wrote:
> > I assume that the cause of the first issue (different key value type) is located here. self.next.GetType().GetByteSize() + self.data_size does not work in this case. Key is read, however value - not. It means that we cannot always add self.data_size.
> > 
> > I tried to manually figure out on the unordered_map<char, int> case what can I add instead of self.data_size (which is 1 in this case), but got no success
> do what i mentioned above regarding fetching the key and value types. I imagine that the internal allocator is using a pair<key, value>, which means that the memory layout will be:
>   key
>   value [with an offset of key size]
here you are returning a child of type self.data_type, which is the key type. And we don't want that. We want to do the same thing that the map formatter does for getting the pair<key, value> and returning it: https://github.com/llvm-mirror/lldb/blob/master/examples/synthetic/gnu_libstdcpp.py#L366
Notice how data type in that case is gotten here https://github.com/llvm-mirror/lldb/blob/master/examples/synthetic/gnu_libstdcpp.py#L367 and is actually the pair. And then they pair is returned here https://github.com/llvm-mirror/lldb/blob/master/examples/synthetic/gnu_libstdcpp.py#L425

that means that you don't need to individually find the key and the value, you only need to find the pair and return the pair

Btw, if raw print an unordered_map, I get this:

(std::unordered_map<int, double, std::hash<int>, std::equal_to<int>, std::allocator<std::pair<const int, double> > >)

that means that the fifth template argument contains the pair. I've confirmed that also libcxx works the same way




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113760