Page MenuHomePhabricator

[lldb] Unify type name matching in FormattersContainer
ClosedPublic

Authored by teemperor on Jul 20 2020, 5:19 AM.

Details

Summary

FormattersContainer stores LLDB's formatters. It's implemented as a templated map-like
data structures that supports any kind of value type and only allows ConstString and RegularExpression
as the key types. The keys are used for matching type names (e.g., the ConstString key std::vector matches
the type with the same name while RegularExpression keys match any type where the RegularExpression
instance matches).

The fact that a single FormattersContainer can only match either by string comparison or regex matching
(depending on the KeyType) causes us to always have two FormatterContainer instances in all the
formatting code. This also leads to us having every type name matching logic in LLDB twice. For example,
TypeCategory has to implement every method twice (one string matching one, one regex matching one).

This patch changes FormattersContainer to instead have a single TypeMatcher key that wraps the logic
for string-based and regex-based type matching and is now the only possible KeyType for the
FormattersContainer. This means that a single FormattersContainer can now match types with both regex
and string comparison.

To summarize the changes in this patch:

  • Remove all the *_Impl methods from FormattersContainer.
  • Instead call the FormatMap functions from FormattersContainer with a TypeMatcher type. that does the respective matching.
  • Replace ConstString with TypeMatcher in the few places that directly interact with FormattersContainer.

I'm working on some follow up patches that I split up because they deserve their own review:

  • Unify FormatMap and FormattersContainer (they are nearly identical now).
  • Delete the duplicated half of all the type matching code that can now use one interface.
  • Propagate TypeMatcher through all the formatter code interfaces instead of always offering two functions for everything.

There is one ugly design part that I couldn't get rid of yet and that is that we have to support getting back
the string used to construct a TypeMatcher later on. The reason for this is that LLDB only supports
referencing existing type matchers by just typing their respective input string again (without even
supplying if it's a regex or not).

Diff Detail

Event Timeline

teemperor created this revision.Jul 20 2020, 5:19 AM
mib added a comment.EditedJul 20 2020, 6:03 AM

The patch looks fine to me. May be it could have a unittest to check the TypeMatcher in an isolated manner ?

lldb/include/lldb/DataFormatters/FormattersContainer.h
46

As you probably saw it on the StackFrameRecognizerManager::RegisteredEntry data structure, it also uses a boolean to check if the recognizer is a regex or not.

Thus for consistency, could you change m_is_str to m_is_regex ?

110

Why not have an equality (==) operator overloading instead ?

teemperor updated this revision to Diff 279518.Jul 21 2020, 7:28 AM
teemperor marked 2 inline comments as done.
  • Added unit tests for new TypeMatcher.
  • m_is_str -> m_is_regex
mib accepted this revision.Jul 21 2020, 9:02 AM

Thanks for the changes. LGTM!

This revision is now accepted and ready to land.Jul 21 2020, 9:02 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2020, 9:46 AM