[Lldb-commits] [PATCH] D134771: [NFCI] Simplify TypeCategoryImpl for-each callbacks.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdee9c7f5d7e5: [NFCI] Simplify TypeCategoryImpl for-each
callbacks. (authored by jgorbe).
Changed prior to commit:
https://reviews.llvm.org/D134771?vs=463624=465824#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134771/new/
https://reviews.llvm.org/D134771
Files:
lldb/include/lldb/DataFormatters/TypeCategory.h
lldb/source/Commands/CommandObjectType.cpp
Index: lldb/source/Commands/CommandObjectType.cpp
===
--- lldb/source/Commands/CommandObjectType.cpp
+++ lldb/source/Commands/CommandObjectType.cpp
@@ -1097,36 +1097,20 @@
"---\nCategory: %s%s\n---\n",
category->GetName(), category->IsEnabled() ? "" : " (disabled)");
- TypeCategoryImpl::ForEachCallbacks foreach;
- foreach
-.SetExact([, _regex, _printed](
- const TypeMatcher _matcher,
- const FormatterSharedPointer _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([, _regex, _printed](
- const TypeMatcher _matcher,
- const FormatterSharedPointer _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 print_formatter =
+ [, _regex,
+ _printed](const TypeMatcher _matcher,
+ const FormatterSharedPointer _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(print_formatter);
};
if (m_options.m_category_language.OptionWasSet()) {
Index: lldb/include/lldb/DataFormatters/TypeCategory.h
===
--- lldb/include/lldb/DataFormatters/TypeCategory.h
+++ lldb/include/lldb/DataFormatters/TypeCategory.h
@@ -130,6 +130,16 @@
return lldb::TypeNameSpecifierImplSP();
}
+ /// Iterates through tiers in order, running `callback` on each element of
+ /// each tier.
+ void ForEach(std::function &)>
+ callback) {
+for (auto sc : m_subcontainers) {
+ sc->ForEach(callback);
+}
+ }
+
private:
std::array, lldb::eLastFormatterMatchType + 1>
m_subcontainers;
@@ -146,120 +156,36 @@
typedef uint16_t FormatCategoryItems;
static const uint16_t ALL_ITEM_TYPES = UINT16_MAX;
- template class ForEachCallbacks {
- public:
-ForEachCallbacks() = default;
-~ForEachCallbacks() = default;
-
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetExact(FormatContainer::ForEachCallback callback) {
- m_format_exact = std::move(callback);
- return *this;
-}
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetWithRegex(FormatContainer::ForEachCallback callback) {
- m_format_regex = std::move(callback);
- return *this;
-}
-
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetExact(SummaryContainer::ForEachCallback callback) {
- m_summary_exact = std::move(callback);
- return *this;
-}
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetWithRegex(SummaryContainer::ForEachCallback callback) {
- m_summary_regex = std::move(callback);
- return *this;
-}
-
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetExact(FilterContainer::ForEachCallback callback) {
- m_filter_exact = std::move(callback);
- return *this;
-}
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetWithRegex(FilterContainer::ForEachCallback callback) {
- m_filter_regex = std::move(callback);
- return *this;
-}
-
-template
-typename
[Lldb-commits] [PATCH] D134771: [NFCI] Simplify TypeCategoryImpl for-each callbacks.
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 ForEachCallback(Callable c) : callback(c) {}
+std::function)>
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
> class CommandObjectTypeFormatterList {
> [...]
> bool DoExecute(...) {
> TypeCategoryImpl::ForEachCallbacks 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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134771: [NFCI] Simplify TypeCategoryImpl for-each callbacks.
jgorbe added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134771/new/ https://reviews.llvm.org/D134771 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134771: [NFCI] Simplify TypeCategoryImpl for-each callbacks.
jgorbe marked an inline comment as done.
jgorbe added inline comments.
Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:136
+ void ForEach(std::function)>
+ 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
class CommandObjectTypeFormatterList {
[...]
bool DoExecute(...) {
TypeCategoryImpl::ForEachCallbacks 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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134771: [NFCI] Simplify TypeCategoryImpl for-each callbacks.
jgorbe updated this revision to Diff 463624.
jgorbe added a comment.
Fixed callback argument type (from `const std::shared_ptr` to
`const std::shared_ptr &`) as suggested by reviewer.
Also use a range-based for loop to loop over subcontainers in
`TieredFormattercontainer::ForEach`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134771/new/
https://reviews.llvm.org/D134771
Files:
lldb/include/lldb/DataFormatters/TypeCategory.h
lldb/source/Commands/CommandObjectType.cpp
Index: lldb/source/Commands/CommandObjectType.cpp
===
--- lldb/source/Commands/CommandObjectType.cpp
+++ lldb/source/Commands/CommandObjectType.cpp
@@ -1097,36 +1097,20 @@
"---\nCategory: %s%s\n---\n",
category->GetName(), category->IsEnabled() ? "" : " (disabled)");
- TypeCategoryImpl::ForEachCallbacks foreach;
- foreach
-.SetExact([, _regex, _printed](
- const TypeMatcher _matcher,
- const FormatterSharedPointer _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([, _regex, _printed](
- const TypeMatcher _matcher,
- const FormatterSharedPointer _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 print_formatter =
+ [, _regex,
+ _printed](const TypeMatcher _matcher,
+ const FormatterSharedPointer _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(print_formatter);
};
if (m_options.m_category_language.OptionWasSet()) {
Index: lldb/include/lldb/DataFormatters/TypeCategory.h
===
--- lldb/include/lldb/DataFormatters/TypeCategory.h
+++ lldb/include/lldb/DataFormatters/TypeCategory.h
@@ -130,6 +130,16 @@
return lldb::TypeNameSpecifierImplSP();
}
+ /// Iterates through tiers in order, running `callback` on each element of
+ /// each tier.
+ void ForEach(std::function &)>
+ callback) {
+for (auto sc : m_subcontainers) {
+ sc->ForEach(callback);
+}
+ }
+
private:
std::array, lldb::eLastFormatterMatchType + 1>
m_subcontainers;
@@ -146,120 +156,35 @@
typedef uint16_t FormatCategoryItems;
static const uint16_t ALL_ITEM_TYPES = UINT16_MAX;
- template class ForEachCallbacks {
- public:
-ForEachCallbacks() = default;
-~ForEachCallbacks() = default;
-
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetExact(FormatContainer::ForEachCallback callback) {
- m_format_exact = std::move(callback);
- return *this;
-}
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetWithRegex(FormatContainer::ForEachCallback callback) {
- m_format_regex = std::move(callback);
- return *this;
-}
-
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetExact(SummaryContainer::ForEachCallback callback) {
- m_summary_exact = std::move(callback);
- return *this;
-}
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetWithRegex(SummaryContainer::ForEachCallback callback) {
- m_summary_regex = std::move(callback);
- return *this;
-}
-
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetExact(FilterContainer::ForEachCallback callback) {
- m_filter_exact = std::move(callback);
- return *this;
-}
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetWithRegex(FilterContainer::ForEachCallback callback) {
- m_filter_regex = std::move(callback);
- return *this;
-}
-
-template
-typename
[Lldb-commits] [PATCH] D134771: [NFCI] Simplify TypeCategoryImpl for-each callbacks.
labath added inline comments.
Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:136
+ void ForEach(std::function)>
+ callback) {
did you want to add a reference here? A const by-value argument is not
particularly useful.
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.
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)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134771/new/
https://reviews.llvm.org/D134771
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D134771: [NFCI] Simplify TypeCategoryImpl for-each callbacks.
jgorbe created this revision.
jgorbe added reviewers: jingham, labath.
jgorbe added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jgorbe requested review of this revision.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D134771
Files:
lldb/include/lldb/DataFormatters/TypeCategory.h
lldb/source/Commands/CommandObjectType.cpp
Index: lldb/source/Commands/CommandObjectType.cpp
===
--- lldb/source/Commands/CommandObjectType.cpp
+++ lldb/source/Commands/CommandObjectType.cpp
@@ -1097,36 +1097,20 @@
"---\nCategory: %s%s\n---\n",
category->GetName(), category->IsEnabled() ? "" : " (disabled)");
- TypeCategoryImpl::ForEachCallbacks foreach;
- foreach
-.SetExact([, _regex, _printed](
- const TypeMatcher _matcher,
- const FormatterSharedPointer _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([, _regex, _printed](
- const TypeMatcher _matcher,
- const FormatterSharedPointer _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 print_formatter =
+ [, _regex,
+ _printed](const TypeMatcher _matcher,
+ const FormatterSharedPointer _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(print_formatter);
};
if (m_options.m_category_language.OptionWasSet()) {
Index: lldb/include/lldb/DataFormatters/TypeCategory.h
===
--- lldb/include/lldb/DataFormatters/TypeCategory.h
+++ lldb/include/lldb/DataFormatters/TypeCategory.h
@@ -130,6 +130,16 @@
return lldb::TypeNameSpecifierImplSP();
}
+ /// Iterates through tiers in order, running `callback` on each element of
+ /// each tier.
+ void ForEach(std::function)>
+ callback) {
+for (int tier = 0; tier <= lldb::eLastFormatterMatchType; ++tier) {
+ m_subcontainers[tier]->ForEach(callback);
+}
+ }
+
private:
std::array, lldb::eLastFormatterMatchType + 1>
m_subcontainers;
@@ -146,120 +156,35 @@
typedef uint16_t FormatCategoryItems;
static const uint16_t ALL_ITEM_TYPES = UINT16_MAX;
- template class ForEachCallbacks {
- public:
-ForEachCallbacks() = default;
-~ForEachCallbacks() = default;
-
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetExact(FormatContainer::ForEachCallback callback) {
- m_format_exact = std::move(callback);
- return *this;
-}
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetWithRegex(FormatContainer::ForEachCallback callback) {
- m_format_regex = std::move(callback);
- return *this;
-}
-
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-SetExact(SummaryContainer::ForEachCallback callback) {
- m_summary_exact = std::move(callback);
- return *this;
-}
-template
-typename std::enable_if::value, ForEachCallbacks &>::type
-
