This is an archive of the discontinued LLVM Phabricator instance.

[NFCI] Simplify TypeCategoryImpl for-each callbacks.
ClosedPublic

Authored by jgorbe on Sep 27 2022, 3:44 PM.

Details

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.

Diff Detail

Event Timeline

jgorbe created this revision.Sep 27 2022, 3:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 3:44 PM
jgorbe requested review of this revision.Sep 27 2022, 3:44 PM
labath added inline comments.Sep 28 2022, 1:01 AM
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–162

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 updated this revision to Diff 463624.Sep 28 2022, 11:05 AM

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 marked an inline comment as done.Sep 28 2022, 11:06 AM
jgorbe added inline comments.
lldb/include/lldb/DataFormatters/TypeCategory.h
136

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

159–162

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 marked an inline comment as done.Sep 28 2022, 11:06 AM
labath accepted this revision.Oct 6 2022, 7:31 AM

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

lldb/include/lldb/DataFormatters/TypeCategory.h
159–162

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.

This revision is now accepted and ready to land.Oct 6 2022, 7:31 AM
This revision was automatically updated to reflect the committed changes.
jgorbe marked an inline comment as done.Oct 6 2022, 12:12 PM