[Formatters][NFCI] Replace 'is_regex' arguments with an enum.

[jgorbe]

Summary

Modify SBTypeNameSpecifier and lldb_private::TypeMatcher so they
have an enum value for the type of matching to perform instead of an
m_is_regex boolean value.

This change paves the way for introducing formatter matching based on
the result of a python callback in addition to the existing name-based
matching. See the RFC thread at
https://discourse.llvm.org/t/rfc-python-callback-for-data-formatters-type-matching/64204
for more details.

[jgorbe]

This is the first step towards something like this big diff: https://reviews.llvm.org/differential/diff/457740/. It's a pretty simple patch, but I would like some feedback about whether you think it's going in the right direction or I should do something else instead.

Here's some more context for that diff linked above. There is, as you could expect, some amount of plumbing so that at callback matching time we have around pointers to the script interpreter, the type of the value, etc, and we can call the callback. But the biggest problem I've found is that there's some logic that is duplicated for each one of {exact, regex} AND for each kind of formatter (summaries, synthetic child providers...). An example of this is TypeCategoryImpl::AnyMatches. Adding a new matching strategy ({exact, regex, callback}) makes the combinatorial problem even worse. So a big chunk of that patch is moving stuff around to try to minimize this problem.

The main refactoring is replacing the existing FormatterContainerPair (that has two members that contain exact match formatters and regex formatters, respectively) with a TieredFormatterContainer which has an array of containers in priority order. The idea is to sink as much of duplicated logic as possible around TypeCategoryImpl (for example, GetFormatAtIndex, GetSummaryAtIndex, GetFilterAtIndex...) into this TieredFormatterContainer to reuse the implementation.

I hope this helps understand the general idea a bit better.

[hawkinsw]

lldb/include/lldb/DataFormatters/FormattersContainer.h

44–48

I am really intrigued by this entire patchset and just wanted to drop by to help, if I could. I hope that I am not upsetting you by commenting on something so trivial, but it seems like we would want to change the comment for this data member while we are at it?

Again, I think this work is great and I hope I am helping!

[jgorbe]

Updated comment above the declaration of TypeMatcher::m_match_type that referred to the old m_is_regex boolean, to address review feedback

[jgorbe]

lldb/include/lldb/DataFormatters/FormattersContainer.h

44–48

Good catch! I have updated the comment. Thanks for pointing it out :)

[jgorbe]

Fixed mis-spelled member name in comment.

[labath]

This seems in line with what was being discussed. Jim, do you have any thoughts on this?

lldb/include/lldb/lldb-enumerations.h

841

I see the enums in this file use wildly inconsistent styles for the "largest value" enumerator. However, you've picked the one I like the least, because this tends to produce unhandled switch case "kNumFormatterMatchTypes" warnings. If you feel like you need to have a sentinel, I'd recommend going with the eLastFormatterMatchType = <last enum value> pattern (used e.g. in the StateType enum).

[jgorbe]

lldb/include/lldb/lldb-enumerations.h

841

I do want a sentinel because the plan is to replace some hardcoded instances of "check for an exact match and if nothing is found then check for a regex match" with a class that has an array of size kNumFormatterMatchTypes formatter containers, and knows how to do a lookup in priority order. So then I can add another "priority tier" for callback matchers and not add extra logic in a bunch of places.

But I have no strong preference between having a std::array<Container, kNumFormatterMatchTypes> and a std::array<Container, eLastFormatterMatchType + 1>, and I agree the unhandled switch case warnings are annoying. I'll change it shortly.

[jgorbe]

Replaced kNumFormatterMatchTypes with eLastFormatterMatchType to avoid "unhandled enum value" warnings, as suggested by Pavel.

lldb/bindings/interface/SBTypeNameSpecifier.i

namespace lldb {
     {
     public:
 
         SBTypeNameSpecifier();
 
         SBTypeNameSpecifier (const char* name,
                              bool is_regex = false);
 
+        SBTypeNameSpecifier (const char* name,
+                             lldb::FormatterMatchType match_type);
+
         SBTypeNameSpecifier (SBType type);
 
         SBTypeNameSpecifier (const lldb::SBTypeNameSpecifier &rhs);
 
         ~SBTypeNameSpecifier ();
 
         bool
         IsValid() const;
 
         explicit operator bool() const;
 
         bool
         IsEqualTo (lldb::SBTypeNameSpecifier &rhs);
 
         const char*
         GetName();
 
         lldb::SBType
         GetType ();
 
+        lldb::FormatterMatchType
+        GetMatchType();
+
         bool
         IsRegex();
 
         bool
         GetDescription (lldb::SBStream &description,
                         lldb::DescriptionLevel description_level);
 
         bool

lldb/include/lldb/API/SBTypeNameSpecifier.h

 namespace lldb {
 
 class LLDB_API SBTypeNameSpecifier {
 public:
   SBTypeNameSpecifier();
 
   SBTypeNameSpecifier(const char *name, bool is_regex = false);
 
+  SBTypeNameSpecifier(const char *name,
+                      lldb::FormatterMatchType match_type);
+
   SBTypeNameSpecifier(SBType type);
 
   SBTypeNameSpecifier(const lldb::SBTypeNameSpecifier &rhs);
 
   ~SBTypeNameSpecifier();
 
   explicit operator bool() const;
 
   bool IsValid() const;
 
   const char *GetName();
 
   SBType GetType();
 
+  lldb::FormatterMatchType GetMatchType();
+
   bool IsRegex();
 
   bool GetDescription(lldb::SBStream &description,
                       lldb::DescriptionLevel description_level);
 
   lldb::SBTypeNameSpecifier &operator=(const lldb::SBTypeNameSpecifier &rhs);
 
   bool IsEqualTo(lldb::SBTypeNameSpecifier &rhs);

lldb/include/lldb/DataFormatters/FormatClasses.h

private:
   ConstString m_type_for_cache;
   CandidateLanguagesVector m_candidate_languages;
 };
 
 class TypeNameSpecifierImpl {
 public:
   TypeNameSpecifierImpl() = default;
 
-  TypeNameSpecifierImpl(llvm::StringRef name, bool is_regex)
-      : m_is_regex(is_regex) {
+  TypeNameSpecifierImpl(llvm::StringRef name,
+                        lldb::FormatterMatchType match_type)
+      : m_match_type(match_type) {
     m_type.m_type_name = std::string(name);
   }
 
-  // if constructing with a given type, is_regex cannot be true since we are
-  // giving an exact type to match
-  TypeNameSpecifierImpl(lldb::TypeSP type) : m_is_regex(false) {
+  // if constructing with a given type, we consider that a case of exact match.
+  TypeNameSpecifierImpl(lldb::TypeSP type)
+      : m_match_type(lldb::eFormatterMatchExact) {
     if (type) {
       m_type.m_type_name = std::string(type->GetName().GetStringRef());
       m_type.m_compiler_type = type->GetForwardCompilerType();
     }
   }
 
-  TypeNameSpecifierImpl(CompilerType type) : m_is_regex(false) {
+  TypeNameSpecifierImpl(CompilerType type)
+      : m_match_type(lldb::eFormatterMatchExact) {
     if (type.IsValid()) {
       m_type.m_type_name.assign(type.GetTypeName().GetCString());
       m_type.m_compiler_type = type;
     }
   }
 
   const char *GetName() {
     if (m_type.m_type_name.size())
       return m_type.m_type_name.c_str();
     return nullptr;
   }
 
   CompilerType GetCompilerType() {
     if (m_type.m_compiler_type.IsValid())
       return m_type.m_compiler_type;
     return CompilerType();
   }
 
-  bool IsRegex() { return m_is_regex; }
+  lldb::FormatterMatchType GetMatchType() { return m_match_type; }
+
+  bool IsRegex() { return m_match_type == lldb::eFormatterMatchRegex; }
 
 private:
-  bool m_is_regex = false;
+  lldb::FormatterMatchType m_match_type = lldb::eFormatterMatchExact;
   // TODO: Replace this with TypeAndOrName.
   struct TypeOrName {
     std::string m_type_name;
     CompilerType m_compiler_type;
   };
   TypeOrName m_type;
 
   TypeNameSpecifierImpl(const TypeNameSpecifierImpl &) = delete;
   const TypeNameSpecifierImpl &
   operator=(const TypeNameSpecifierImpl &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_DATAFORMATTERS_FORMATCLASSES_H

lldb/include/lldb/DataFormatters/FormattersContainer.h

public:
 
   virtual uint32_t GetCurrentRevision() = 0;
 };
 
 /// Class for matching type names.
 class TypeMatcher {
   RegularExpression m_type_name_regex;
   ConstString m_type_name;
-  /// False if m_type_name_regex should be used for matching. False if this is
-  /// just matching by comparing with m_type_name string.
-  bool m_is_regex;
+  /// Indicates what kind of matching strategy should be used:
+  /// - eFormatterMatchExact: match the exact type name in m_type_name.
+  /// - eFormatterMatchRegex: match using the RegularExpression object
+  ///   `m_type_name_regex` instead.
+  lldb::FormatterMatchType m_match_type;
 
   // if the user tries to add formatters for, say, "struct Foo" those will not
   // match any type because of the way we strip qualifiers from typenames this
   // method looks for the case where the user is adding a
   // "class","struct","enum" or "union" Foo and strips the unnecessary qualifier
   static ConstString StripTypeName(ConstString type) {
     if (type.IsEmpty())
       return type;
static ConstString StripTypeName(ConstString type) {
 
     return ConstString(type_lexer.GetUnlexed());
   }
 
 public:
   TypeMatcher() = delete;
   /// Creates a matcher that accepts any type with exactly the given type name.
   TypeMatcher(ConstString type_name)
-      : m_type_name(type_name), m_is_regex(false) {}
+      : m_type_name(type_name), m_match_type(lldb::eFormatterMatchExact) {}
   /// Creates a matcher that accepts any type matching the given regex.
   TypeMatcher(RegularExpression regex)
-      : m_type_name_regex(std::move(regex)), m_is_regex(true) {}
+      : m_type_name_regex(std::move(regex)),
+        m_match_type(lldb::eFormatterMatchRegex) {}
 
   /// True iff this matches the given type name.
   bool Matches(ConstString type_name) const {
-    if (m_is_regex)
+    if (m_match_type == lldb::eFormatterMatchRegex)
       return m_type_name_regex.Execute(type_name.GetStringRef());
     return m_type_name == type_name ||
            StripTypeName(m_type_name) == StripTypeName(type_name);
   }
 
+  lldb::FormatterMatchType GetMatchType() const { return m_match_type; }
+
   /// Returns the underlying match string for this TypeMatcher.
   ConstString GetMatchString() const {
-    if (m_is_regex)
+    if (m_match_type == lldb::eFormatterMatchRegex)
       return ConstString(m_type_name_regex.GetText());
     return StripTypeName(m_type_name);
   }
 
   /// Returns true if this TypeMatcher and the given one were most created by
   /// the same match string.
   /// The main purpose of this function is to find existing TypeMatcher
   /// instances by the user input that created them. This is necessary as LLDB
public:
   }
 
   lldb::TypeNameSpecifierImplSP GetTypeNameSpecifierAtIndex(size_t index) {
     std::lock_guard<std::recursive_mutex> guard(m_map_mutex);
     if (index >= m_map.size())
       return lldb::TypeNameSpecifierImplSP();
     TypeMatcher type_matcher = m_map[index].first;
     return std::make_shared<TypeNameSpecifierImpl>(
-        type_matcher.GetMatchString().GetStringRef(), true);
+        type_matcher.GetMatchString().GetStringRef(),
+        type_matcher.GetMatchType());
   }
 
   void Clear() {
     std::lock_guard<std::recursive_mutex> guard(m_map_mutex);
     m_map.clear();
     if (listener)
       listener->Changed();
   }

lldb/include/lldb/lldb-enumerations.h

enum TemplateArgumentKind {
   eTemplateArgumentKindIntegral,
   eTemplateArgumentKindTemplate,
   eTemplateArgumentKindTemplateExpansion,
   eTemplateArgumentKindExpression,
   eTemplateArgumentKindPack,
   eTemplateArgumentKindNullPtr,
 };
 
+/// Type of match to be performed when looking for a formatter for a data type.
+/// Used by classes like SBTypeNameSpecifier or lldb_private::TypeMatcher.
+enum FormatterMatchType {
+  eFormatterMatchExact,
+  eFormatterMatchRegex,
+
+  eLastFormatterMatchType = eFormatterMatchRegex,
+};
+
 /// Options that can be set for a formatter to alter its behavior. Not
 /// all of these are applicable to all formatter types.
 FLAGS_ENUM(TypeOptions){eTypeOptionNone = (0u),
                         eTypeOptionCascade = (1u << 0),
                         eTypeOptionSkipPointers = (1u << 1),
                         eTypeOptionSkipReferences = (1u << 2),
                         eTypeOptionHideChildren = (1u << 3),
                         eTypeOptionHideValue = (1u << 4),

lldb/source/API/SBTypeNameSpecifier.cpp

 #include "lldb/DataFormatters/DataVisualization.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 SBTypeNameSpecifier::SBTypeNameSpecifier() { LLDB_INSTRUMENT_VA(this); }
 
 SBTypeNameSpecifier::SBTypeNameSpecifier(const char *name, bool is_regex)
-    : m_opaque_sp(new TypeNameSpecifierImpl(name, is_regex)) {
+    : SBTypeNameSpecifier(name, is_regex ? eFormatterMatchRegex
+                                         : eFormatterMatchExact) {
   LLDB_INSTRUMENT_VA(this, name, is_regex);
+}
+
+SBTypeNameSpecifier::SBTypeNameSpecifier(const char *name,
+                                         FormatterMatchType match_type)
+    : m_opaque_sp(new TypeNameSpecifierImpl(name, match_type)) {
+  LLDB_INSTRUMENT_VA(this, name, match_type);
 
   if (name == nullptr || (*name) == 0)
     m_opaque_sp.reset();
 }
 
 SBTypeNameSpecifier::SBTypeNameSpecifier(SBType type) {
   LLDB_INSTRUMENT_VA(this, type);
 
SBType SBTypeNameSpecifier::GetType() {
   if (!IsValid())
     return SBType();
   lldb_private::CompilerType c_type = m_opaque_sp->GetCompilerType();
   if (c_type.IsValid())
     return SBType(c_type);
   return SBType();
 }
 
+FormatterMatchType SBTypeNameSpecifier::GetMatchType() {
+  LLDB_INSTRUMENT_VA(this);
+  if (!IsValid())
+    return eFormatterMatchExact;
+  return m_opaque_sp->GetMatchType();
+}
+
 bool SBTypeNameSpecifier::IsRegex() {
   LLDB_INSTRUMENT_VA(this);
 
   if (!IsValid())
     return false;
 
-  return m_opaque_sp->IsRegex();
+  return m_opaque_sp->GetMatchType() == eFormatterMatchRegex;
 }
 
 bool SBTypeNameSpecifier::GetDescription(
     lldb::SBStream &description, lldb::DescriptionLevel description_level) {
   LLDB_INSTRUMENT_VA(this, description, description_level);
 
   if (!IsValid())
     return false;

 }
 
 bool SBTypeNameSpecifier::IsEqualTo(lldb::SBTypeNameSpecifier &rhs) {
   LLDB_INSTRUMENT_VA(this, rhs);
 
   if (!IsValid())
     return !rhs.IsValid();
 
-  if (IsRegex() != rhs.IsRegex())
+  if (GetMatchType() != rhs.GetMatchType())
     return false;
   if (GetName() == nullptr || rhs.GetName() == nullptr)
     return false;
 
   return (strcmp(GetName(), rhs.GetName()) == 0);
 }
 
 bool SBTypeNameSpecifier::operator!=(lldb::SBTypeNameSpecifier &rhs) {

lldb/source/Core/FormatEntity.cpp

static bool DumpValue(Stream &s, const SymbolContext *sc,
   // originally dealing with just a T - that would get us into an endless
   // recursion
   if (target->IsBitfield() && was_var_indexed) {
     // TODO: check for a (T:n)-specific summary - we should still obey that
     StreamString bitfield_name;
     bitfield_name.Printf("%s:%d", target->GetTypeName().AsCString(),
                          target->GetBitfieldBitSize());
     auto type_sp = std::make_shared<TypeNameSpecifierImpl>(
-        bitfield_name.GetString(), false);
+        bitfield_name.GetString(), lldb::eFormatterMatchExact);
     if (val_obj_display ==
             ValueObject::eValueObjectRepresentationStyleSummary &&
         !DataVisualization::GetSummaryForType(type_sp))
       val_obj_display = ValueObject::eValueObjectRepresentationStyleValue;
   }
 
   // TODO use flags for these
   const uint32_t type_info_flags =