[Lldb-commits] [PATCH] D113760: [formatters] Add a libstdcpp formatter for for unordered_map, unordered_set, unordered_multimap, unordered_multiset

[walter erquinigo via Phabricator via lldb-commits]

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

just some minor changes.
Could you unify these tests with libcxx?



================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:10
 # before relying on these formatters to do the right thing for your setup
+class StdUnorderedMapSynthProvider:
+    def __init__(self, valobj, dict):
----------------
add a few empty lines before this to avoid confusing that comment with this specific formatter. Besides that, rename this to StdUnorderedMapLikeSynthProvider and add a comment similar to 

    """
     This formatter can be applied to all
     unordered map-like structures (unordered_map, unordered_multimap, unordered_set, unordered_multiset)
    """




================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:25
+              return kind
+        return Exception
+
----------------
an exception is too much. Normally we just fail silently, as it obvious to the user when the formatter fails and investigation is needed. Just return "map" if "set" is not present in the type name.


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:29
+        type = self.valobj.GetType()
+        template_arg_num = 4 if self.kind == "map" else 3
+        allocator_type = type.GetTemplateArgumentType(template_arg_num)
----------------
this comment

        # type of std::pair<key, value> is the first template
        # argument type of the 4th template argument to std::map and 
        # 3rd template argument for std::set. That's why 
        # we need to know kind of the object

should be here


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:32
+        data_type = allocator_type.GetTemplateArgumentType(0)
+        return data_type
+
----------------
if we used something that is not a map or set, this data type will be a dummy object that won't fail but won't do anything useful


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:46
+        except:
+            pass
+        return False
----------------
you see? we fail silently by default


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:83
+            count = self.head.GetChildMemberWithName('_M_element_count').GetValueAsUnsigned(0)
+            logger >> "I have " + str(count) + " children available"
+            return count
----------------
remove this comment, as this information is easily available to the user


================
Comment at: lldb/examples/synthetic/gnu_libstdcpp.py:86
+        except:
+            logger >> "Could not determine the size"
+            return 0
----------------
this one is fine


================
Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/main.cpp:5
+
+using std::string;
+
----------------
also remove this, you barely save a few characters by not typing std::string


================
Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/main.cpp:7-13
+#define intstr_map std::unordered_map<int, string>
+#define intstr_mmap std::unordered_multimap<int, string>
+
+#define int_set std::unordered_set<int>
+#define str_set std::unordered_set<string>
+#define int_mset std::unordered_multiset<int>
+#define str_mset std::unordered_multiset<string>
----------------
avoid using these defines, they just obscure the code when reading


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113760