[NFCI] Simplify TypeCategoryImpl for-each callbacks.

[jgorbe]

Summary

The callback system to iterate over every formatter of a given kind in
a TypeCategoryImpl is only used in one place (the implementation of
type {formatter_kind} list), and it's too convoluted for the sake of
unused flexibility.

This change changes it so that only one callback is passed to ForEach
(instead of a callback for exact matches and another one for regex
matches), and moves the iteration logic to TieredFormatterContainer
to avoid duplication.

If in the future we need different logic in the callback depending on
exact/regex match, the callback can get the type of formatter matching
used from the TypeMatcher argument anyway.

[labath]

lldb/include/lldb/DataFormatters/TypeCategory.h

136

did you want to add a reference here? A const by-value argument is not particularly useful.

159–170

What if we just embed the type information into the method name? (So we could have a set of ForEachFormat,ForEachSummary, ... methods instead of a single overloaded ForEach)

[jgorbe]

Fixed callback argument type (from const std::shared_ptr<FormatterImpl> to const std::shared_ptr<FormatterImpl> &) as suggested by reviewer.

Also use a range-based for loop to loop over subcontainers in TieredFormattercontainer::ForEach.

[jgorbe]

lldb/include/lldb/DataFormatters/TypeCategory.h

136

Yeah, I just changed it. Thanks for catching it!

159–170

The problem with that is that the call site is

template <typename FormatterType>
class CommandObjectTypeFormatterList {
  [...]
  bool DoExecute(...) {
      TypeCategoryImpl::ForEachCallbacks<FormatterType> foreach;
      category->ForEach(foreach);

So if we want to keep that template for CommandObjectTypeFormatterList to avoid repeating the implementation of type {format, summary, filter, synthetic} list, we still need to switch based on type somewhere. So it might as well be here. Or did you have anything else in mind?

[jgorbe]

Ping.

[labath]

(it looks like I did not click "submit" for some reason)

lldb/include/lldb/DataFormatters/TypeCategory.h

159–170

No, this is what I had in mind, but I somehow missed the fact that the call site is templated. Ok, let's stick to this then.

169

And one reference here as well.

lldb/include/lldb/DataFormatters/TypeCategory.h

lldb::TypeNameSpecifierImplSP GetTypeNameSpecifierAtIndex(size_t index) {
     for (auto sc : m_subcontainers) {
       if (index < sc->GetCount())
         return sc->GetTypeNameSpecifierAtIndex(index);
       index -= sc->GetCount();
     }
     return lldb::TypeNameSpecifierImplSP();
   }
 
+  /// Iterates through tiers in order, running `callback` on each element of
+  /// each tier.
+  void ForEach(std::function<bool(const TypeMatcher &,
+                                  const std::shared_ptr<FormatterImpl> &)>
+                   callback) {
+    for (auto sc : m_subcontainers) {
+      sc->ForEach(callback);
+    }
+  }
+
  private:
   std::array<std::shared_ptr<Subcontainer>, lldb::eLastFormatterMatchType + 1>
       m_subcontainers;
 };
 
 class TypeCategoryImpl {
 private:
   typedef TieredFormatterContainer<TypeFormatImpl> FormatContainer;
   typedef TieredFormatterContainer<TypeSummaryImpl> SummaryContainer;
   typedef TieredFormatterContainer<TypeFilterImpl> FilterContainer;
   typedef TieredFormatterContainer<SyntheticChildren> SynthContainer;
 
 public:
   typedef uint16_t FormatCategoryItems;
   static const uint16_t ALL_ITEM_TYPES = UINT16_MAX;
 
-  template <typename T> class ForEachCallbacks {
-  public:
-    ForEachCallbacks() = default;
-    ~ForEachCallbacks() = default;
-
-    template <typename U = TypeFormatImpl>
-    typename std::enable_if<std::is_same<U, T>::value, ForEachCallbacks &>::type
-    SetExact(FormatContainer::ForEachCallback callback) {
-      m_format_exact = std::move(callback);
-      return *this;
-    }
-    template <typename U = TypeFormatImpl>
-    typename std::enable_if<std::is_same<U, T>::value, ForEachCallbacks &>::type
-    SetWithRegex(FormatContainer::ForEachCallback callback) {
-      m_format_regex = std::move(callback);
-      return *this;
-    }
-
-    template <typename U = TypeSummaryImpl>
-    typename std::enable_if<std::is_same<U, T>::value, ForEachCallbacks &>::type
-    SetExact(SummaryContainer::ForEachCallback callback) {
-      m_summary_exact = std::move(callback);
-      return *this;
-    }
-    template <typename U = TypeSummaryImpl>
-    typename std::enable_if<std::is_same<U, T>::value, ForEachCallbacks &>::type
-    SetWithRegex(SummaryContainer::ForEachCallback callback) {
-      m_summary_regex = std::move(callback);
-      return *this;
-    }
-
-    template <typename U = TypeFilterImpl>
-    typename std::enable_if<std::is_same<U, T>::value, ForEachCallbacks &>::type
-    SetExact(FilterContainer::ForEachCallback callback) {
-      m_filter_exact = std::move(callback);
-      return *this;
-    }
-    template <typename U = TypeFilterImpl>
-    typename std::enable_if<std::is_same<U, T>::value, ForEachCallbacks &>::type
-    SetWithRegex(FilterContainer::ForEachCallback callback) {
-      m_filter_regex = std::move(callback);
-      return *this;
-    }
-
-    template <typename U = SyntheticChildren>
-    typename std::enable_if<std::is_same<U, T>::value, ForEachCallbacks &>::type
-    SetExact(SynthContainer::ForEachCallback callback) {
-      m_synth_exact = std::move(callback);
-      return *this;
-    }
-    template <typename U = SyntheticChildren>
-    typename std::enable_if<std::is_same<U, T>::value, ForEachCallbacks &>::type
-    SetWithRegex(SynthContainer::ForEachCallback callback) {
-      m_synth_regex = std::move(callback);
-      return *this;
-    }
+  // TypeFilterImpl inherits from SyntheticChildren, so we can't simply overload
+  // ForEach on the type of the callback because it would result in "call to
+  // member function 'ForEach' is ambiguous" errors. Instead we use this
+  // templated struct to hold the formatter type and the callback.
+  template<typename T>
+  struct ForEachCallback {
+    // Make it constructible from any callable that fits. This allows us to use
+    // lambdas a bit more easily at the call site. For example:
+    // ForEachCallback<TypeFormatImpl> callback = [](...) {...};
+    template <typename Callable> ForEachCallback(Callable c) : callback(c) {}
+    std::function<bool(const TypeMatcher &, const std::shared_ptr<T>)> callback;
+  };
 
-    FormatContainer::ForEachCallback GetFormatExactCallback() const {
-      return m_format_exact;
-    }
-    FormatContainer::ForEachCallback GetFormatRegexCallback() const {
-      return m_format_regex;
-    }
+  TypeCategoryImpl(IFormatChangeListener *clist, ConstString name);
 
-    SummaryContainer::ForEachCallback GetSummaryExactCallback() const {
-      return m_summary_exact;
-    }
-    SummaryContainer::ForEachCallback GetSummaryRegexCallback() const {
-      return m_summary_regex;
-    }
+  void ForEach(ForEachCallback<TypeFormatImpl> callback) {
+    m_format_cont.ForEach(callback.callback);
 
-    FilterContainer::ForEachCallback GetFilterExactCallback() const {
-      return m_filter_exact;
-    }
-    FilterContainer::ForEachCallback GetFilterRegexCallback() const {
-      return m_filter_regex;
-    }
+  void ForEach(ForEachCallback<TypeSummaryImpl> callback) {
+    m_summary_cont.ForEach(callback.callback);
 
-    SynthContainer::ForEachCallback GetSynthExactCallback() const {
-      return m_synth_exact;
-    }
-    SynthContainer::ForEachCallback GetSynthRegexCallback() const {
-      return m_synth_regex;
-    }
+  void ForEach(ForEachCallback<TypeFilterImpl> callback) {
+    m_filter_cont.ForEach(callback.callback);
 
-  private:
-    FormatContainer::ForEachCallback m_format_exact;
-    FormatContainer::ForEachCallback m_format_regex;
-
-    SummaryContainer::ForEachCallback m_summary_exact;
-    SummaryContainer::ForEachCallback m_summary_regex;
-
-    FilterContainer::ForEachCallback m_filter_exact;
-    FilterContainer::ForEachCallback m_filter_regex;
-
-    SynthContainer::ForEachCallback m_synth_exact;
-    SynthContainer::ForEachCallback m_synth_regex;
-  };
-
-  TypeCategoryImpl(IFormatChangeListener *clist, ConstString name);
-
-  template <typename T> void ForEach(const ForEachCallbacks<T> &foreach) {
-    GetTypeFormatsContainer()->ForEach(foreach.GetFormatExactCallback());
-    GetRegexTypeFormatsContainer()->ForEach(foreach.GetFormatRegexCallback());
-
-    GetTypeSummariesContainer()->ForEach(foreach.GetSummaryExactCallback());
-    GetRegexTypeSummariesContainer()->ForEach(
-        foreach.GetSummaryRegexCallback());
-
-    GetTypeFiltersContainer()->ForEach(foreach.GetFilterExactCallback());
-    GetRegexTypeFiltersContainer()->ForEach(foreach.GetFilterRegexCallback());
-
-    GetTypeSyntheticsContainer()->ForEach(foreach.GetSynthExactCallback());
-    GetRegexTypeSyntheticsContainer()->ForEach(foreach.GetSynthRegexCallback());
+  void ForEach(ForEachCallback<SyntheticChildren> callback) {
+    m_synth_cont.ForEach(callback.callback);
   }
 
   FormatContainer::SubcontainerSP GetTypeFormatsContainer() {
     return m_format_cont.GetExactMatch();
   }
 
   FormatContainer::SubcontainerSP GetRegexTypeFormatsContainer() {
     return m_format_cont.GetRegexMatch();

lldb/source/Commands/CommandObjectType.cpp

bool DoExecute(Args &command, CommandReturnObject &result) override {
 
     auto category_closure =
         [&result, &formatter_regex,
          &any_printed](const lldb::TypeCategoryImplSP &category) -> void {
       result.GetOutputStream().Printf(
           "-----------------------\nCategory: %s%s\n-----------------------\n",
           category->GetName(), category->IsEnabled() ? "" : " (disabled)");
 
-      TypeCategoryImpl::ForEachCallbacks<FormatterType> foreach;
-      foreach
-        .SetExact([&result, &formatter_regex, &any_printed](
-                      const TypeMatcher &type_matcher,
-                      const FormatterSharedPointer &format_sp) -> bool {
-          if (ShouldListItem(type_matcher.GetMatchString().GetStringRef(),
-                             formatter_regex.get())) {
-            any_printed = true;
-            result.GetOutputStream().Printf(
-                "%s: %s\n", type_matcher.GetMatchString().GetCString(),
-                format_sp->GetDescription().c_str());
-          }
-          return true;
-        });
-
-      foreach
-        .SetWithRegex([&result, &formatter_regex, &any_printed](
-                          const TypeMatcher &type_matcher,
-                          const FormatterSharedPointer &format_sp) -> bool {
-          if (ShouldListItem(type_matcher.GetMatchString().GetStringRef(),
-                             formatter_regex.get())) {
-            any_printed = true;
-            result.GetOutputStream().Printf(
-                "%s: %s\n", type_matcher.GetMatchString().GetCString(),
-                format_sp->GetDescription().c_str());
-          }
-          return true;
-        });
-
-      category->ForEach(foreach);
+      TypeCategoryImpl::ForEachCallback<FormatterType> print_formatter =
+          [&result, &formatter_regex,
+           &any_printed](const TypeMatcher &type_matcher,
+      };
+      category->ForEach(print_formatter);
     };
 
     if (m_options.m_category_language.OptionWasSet()) {
       lldb::TypeCategoryImplSP category_sp;
       DataVisualization::Categories::GetCategory(
           m_options.m_category_language.GetCurrentValue(), category_sp);
       if (category_sp)
         category_closure(category_sp);