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

[Jorge Gorbe Moya via Phabricator via lldb-commits]

jgorbe marked an inline comment as done.
jgorbe added inline comments.


================
Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:136
+  void ForEach(std::function<bool(const TypeMatcher &,
+                                  const std::shared_ptr<FormatterImpl>)>
+                   callback) {
----------------
labath wrote:
> did you want to add a reference here? A const by-value argument is not particularly useful.
Yeah, I just changed it. Thanks for catching it!


================
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.
----------------
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?


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

https://reviews.llvm.org/D134771