[Lldb-commits] [PATCH] D134771: [NFCI] Simplify TypeCategoryImpl for-each callbacks.

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:169
+    template <typename Callable> ForEachCallback(Callable c) : callback(c) {}
+    std::function<bool(const TypeMatcher &, const std::shared_ptr<T>)> callback;
   };
----------------
And one reference here as well.


================
Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:159-162
+  // 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.
----------------
jgorbe wrote:
> labath wrote:
> > 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)
> 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?
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.


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

https://reviews.llvm.org/D134771