[Lldb-commits] [PATCH] D112752: [formatters] Add a libstdcpp formatter for multimap and unify modify tests across stdlibs

[walter erquinigo via Phabricator via lldb-commits]

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

pretty good, just cosmetic changes and a comment update needed



================
Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:915
+          stl_deref_flags,
+          "lldb.formatters.cpp.gnu_libstdcpp.StdSetOrMapSynthProvider")));
   cpp_category_sp->GetRegexTypeSyntheticsContainer()->Add(
----------------
Now I don't like the name of the class. Let's rename it to MakeLikeSynthProvider, and update the comments mentioning that it supports any map-like structure like map, multimap, set and multiset.


================
Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/TestDataFormatterGenericMultiMap.py:40
+            children.append(ValueCheck(value=child.GetValue()))
+        self.expect_var_path(var_name, type=self.getVariableType(var_name), children=children)
+
----------------
nice!!


================
Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/TestDataFormatterGenericMultiMap.py:88-285
+        self.check("ii",2)
 
         lldbutil.continue_to_breakpoint(self.process(), bkpt)
 
         self.expect('frame variable ii',
                     substrs=[multimap, 'size=4',
                              '[2] = ',
----------------
the general practice is to leave a space after each comma for readability, e.g.
  self.check("ss", 3)
instead of
  self.check("ss",3)

these little details matter as it help people more easily distinguish the end of each token


================
Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/TestDataFormatterGenericMultiMap.py:320
+
+        self.check("ss",0)
+
----------------
same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112752