[lldb] Refactor ObjCLanguage::MethodName

[bulbazord]

Summary

The goal of this patch is to make it easier to reason about the state of
ObjCLanguage::MethodName. I do that in several ways:

• Instead of using the constructor directly, you go through a factory method. It returns a std::optional<MethodName> so either you got an ObjCLanguage::MethodName or you didn't. No more checking if it's valid to know if you can use it or not.
• ObjCLanguage::MethodName is now immutable. You cannot change its internals once it is created.
• ObjCLanguage::MethodName::GetFullNameWithoutCategory previously had a parameter that let you get back an empty string if the method had no category. Every caller of this method was enabling this behavior so I dropped the parameter and made it the default behavior.
• No longer store all the various components of the method name as ConstStrings. The relevant Get methods now return llvm::StringRefs backed by the MethodName's internal storage. The lifetime of these StringRefs are tied to the MethodName itself, so if you need to persist these you need to create copies.

[jingham]

Most of this is fine. I wonder about avoiding caching the full name and name w/o category & selector name. One of the main uses of this class is to take incoming ObjC names from the ConstString pool, chop them up into full name, name w/o category, and selectorName, which we then insert into the lookup names tables for the modules they are found in. All those lookup table names will also exist in the ConstString pool.

The other instance where we get these names is when someone passes them to break set. In that case, we're going to break the name apart and then look it and its parts up in the ConstString lookup tables to find a match.

So it seems like this is a case where you aren't really getting savings in the ConstString pool, you're just causing more copying from std::string to ConstString.

lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

37

100

What does this return if there's no category?

We have two behaviors above:

GetFullNameWithoutCategory - returns an empty string if the original class had no category
GetClassNameWithCategory - returns the original class name if it had no category
GetCategory - returns ??? if it had no category.

These seem a little skew somehow...

117

For the most part, we find ObjC method names either from debug info, from the symbol table or from the runtime symbol vendor. All of those are going to store their names in the ConstString pool.

If we only make these MethodName entities one by one then we're just doing a string copy every so often, and that shouldn't be noticeable. But if we make and hold a lot of these, we're doubling our storage requirements for a thing that at least on Darwin there are a lot of...

This one seems a reasonable candidate to be a ConstString...

[bulbazord]

In D149914#4331048, @jingham wrote:

Most of this is fine. I wonder about avoiding caching the full name and name w/o category & selector name. One of the main uses of this class is to take incoming ObjC names from the ConstString pool, chop them up into full name, name w/o category, and selectorName, which we then insert into the lookup names tables for the modules they are found in. All those lookup table names will also exist in the ConstString pool.

The full name being a ConstString might make sense since we're usually getting them from debug info or similar sources. When we chop them up, we can decide if it's worth making ConstStrings out of them. At least that's the idea.
That being said, I'll look in more detail about how often we actually try to create ObjCLanguage::MethodName objects. If we do it in a hot loop then maybe ConstString is the right thing to do.

The other instance where we get these names is when someone passes them to break set. In that case, we're going to break the name apart and then look it and its parts up in the ConstString lookup tables to find a match.

So it seems like this is a case where you aren't really getting savings in the ConstString pool, you're just causing more copying from std::string to ConstString.

What's the code path for this? I'm not 100% convinced we should be automatically storing portions of user input in the ConstString StringPool until we have a little more information about what they gave us.

lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

37

Good catch, thanks!

100

GetCategory will return an empty StringRef if there is no category. I will explicitly document that.

I agree that there's some skew here though. The way I think about it is GetCategory and GetClassNameWithCategory do what they say they will do. If there's no category, both of these will leave out the category. The former will therefore be empty and the latter will give you what you want without the category. Maybe it could be empty though? I haven't thought about it.

117

Oh, I suppose that's true. I was looking at the call-sites and a lot of them were passing in const char *, but those likely come from ConstStrings at some layer. In that case, making m_full be a ConstString is probably fine.

BTW, from what I can tell, we only do make these ObjCLanguage::MethodName entities one-by-one. They are never really stored anywhere, they're mostly used to chop up things we think may be ObjCLanguage method names and get the parts out we find useful. Aside from the input, I wanted to create an interface where the caller can decide if the output of its functions should be stored in a ConstString or not instead of preemptively putting it in the StringPool.

[bulbazord]

Ok, I'm back with actual data on this patch. I have a test ObjC app with some UI, it loads a couple hundred modules. I place a breakpoint on main, I run until we hit main, and then I continue. My app will stop at -[NSBlockOperation main] after loading all the required modules. Here is the data without this patch applied:

% hyperfine -w 3 -- "$lldb -x -b -o 'b main' -o 'r' -o 'c' $App"
Benchmark 1: $lldb -x -b -o 'b main' -o 'r' -o 'c' $App
  Time (mean ± σ):      6.367 s ±  0.037 s    [User: 5.688 s, System: 0.295 s]
  Range (min … max):    6.312 s …  6.417 s    10 runs

...

  "memory": {
    "strings": {
      "bytesTotal": 133180053,
      "bytesUnused": 52050681,
      "bytesUsed": 81129372
    }
  },

Now, with the patch applied:

% hyperfine -w 3 -- "$lldb -x -b -o 'b main' -o 'r' -o 'c' $App"
Benchmark 1: $lldb -x -b -o 'b main' -o 'r' -o 'c' $App
  Time (mean ± σ):      6.209 s ±  0.046 s    [User: 5.554 s, System: 0.282 s]
  Range (min … max):    6.159 s …  6.295 s    10 runs

...

  "memory": {
    "strings": {
      "bytesTotal": 133175645,
      "bytesUnused": 52115081,
      "bytesUsed": 81060564
    }
  },

From this we can see that my patch actually is _slightly_ faster, saving a little more than 100ms on average. I ran hyperfine a few times under the same conditions and this is quite repeatable.
As for memory consumption, the ConstString StringPool is slightly smaller. The amount of bytes actually used in the StringPool shrank by about 60kb, and the total size of the StringPool shrank by about 5kb -- meaning we stored a lot less strings and grew the StringPool slightly less than before.

Now we had some discussion about ObjCLanguage::MethodName using a ConstString instead of a std::string to back its m_full member. I gathered the numbers on that one too:

% hyperfine -w 3 -- "$lldb -x -b -o 'b main' -o 'r' -o 'c' $App"
Benchmark 1: $lldb -x -b -o 'b main' -o 'r' -o 'c' $App
  Time (mean ± σ):      6.327 s ±  0.023 s    [User: 5.653 s, System: 0.296 s]
  Range (min … max):    6.299 s …  6.361 s    10 runs

...

  "memory": {
    "strings": {
      "bytesTotal": 133175645,
      "bytesUnused": 52115081,
      "bytesUsed": 81060564
    }
  },

From this, it looks like this is probably ever so slightly faster than with no patch, but still not as fast as using std::string. The memory consumption numbers are the exact same as using std::string. The conclusion I'm drawing from all the data here is that: 1.) My patch does decrease the size of the ConstString StringPool regardless of whether or not m_full is a std::string or a ConstString, and 2.) the std::string implementation is slightly faster on average than the ConstString one. I'll probably stick to this implementation since it appears to be faster on average (at least on my machine), but I'll update this PR to fix the documentation.

[bulbazord]

Update documentation for clarity

[aprantl]

I'm not opposed to using this implementation, but have you considered using something like the stdlib regex library to do the heavy lifting?

[bulbazord]

In D149914#4335858, @aprantl wrote:

I'm not opposed to using this implementation, but have you considered using something like the stdlib regex library to do the heavy lifting?

I had not considered it actually... I don't have any experience using anything from <regex>. I know LLVM has its own regex engine, perhaps that might be appropriate? I know LLDB uses it in its RegularExpression class.

[jingham]

Your test measured setting a found simple breakpoint. That should measure filling all the names caches - we do that the first time you try to set a breakpoint of any sort. But doesn't measure the effects on lookup. I am guessing you will find the same "not much difference" here as well, but it would be good to test that. So it would be good to also ensure you aren't slowing down looking for a selector by name, and looking for a selector you aren't going to find by name, and looking by full ObjC name. But if that's also true, then I'm fine with this.

[bulbazord]

Some more numbers:

% hyperfine -w 3 -- "$lldb -x -b -o 'b main' -o 'r' -o 'c' -o 'b $Selector' -o 'b $NonExistentSelector' -o 'b $FullObjCName' $App"
Benchmark 1: $lldb -x -b -o 'b main' -o 'r' -o 'c' -o 'b $Selector' -o 'b $NonExistentSelector' -o 'b $FullObjCName' $App
  Time (mean ± σ):      6.323 s ±  0.069 s    [User: 5.626 s, System: 0.301 s]
  Range (min … max):    6.224 s …  6.443 s    10 runs

% hyperfine -w 3 -- "$lldb -x -b -o 'b main' -o 'r' -o 'c' -o 'b $Selector' -o 'b $NonExistentSelector' -o 'b $FullObjCName' $App"
Benchmark 1: $lldb -x -b -o 'b main' -o 'r' -o 'c' -o 'b $Selector' -o 'b $NonExistentSelector' -o 'b $FullObjCName' $App
  Time (mean ± σ):      6.270 s ±  0.042 s    [User: 5.557 s, System: 0.313 s]
  Range (min … max):    6.210 s …  6.338 s    10 runs

I hope this is sufficient to show we're not regressing performance. I measured this a few times with and without my change and I observed that this is usually faster but they are usually within 100ms of each other.

[jingham]

LGTM, Adrian's comment is still outstanding however.

[bulbazord]

In D149914#4335858, @aprantl wrote:

I'm not opposed to using this implementation, but have you considered using something like the stdlib regex library to do the heavy lifting?

I talked to Jonas and did a little research. It seems like <regex> is quite slow and many supported compilers have buggy implementations. If I were to use regular expressions to rewrite this, I'd likely use whatever LLVM has implemented. That being said, I'm not too keen on holding up this patch because of that.

[aprantl]

One more comment inline but otherwise this is fine with me.

lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp

155

Using an llvm string stream to construct the string might be faster here.

[bulbazord]

lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp

155

I tested this and found that llvm::SmallString<128> performs about as well as std::string on average. llvm::SmallString<64> was on average slower though.

I think I'm going to land this with std::string, we can revisit later if the difference actually is measurable.

[aprantl]

lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp

155

Maybe we were talking past each other: I was thinking about https://www.llvm.org/doxygen/classllvm_1_1raw__string__ostream.html

[bulbazord]

lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp

155

Oh, I totally misread what you said. I'll try that out and create a new PR if I can improve this.

[bulbazord]

lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp

155

Turns out it can be improved, but it wasn't with raw_string_ostream! https://reviews.llvm.org/D150914

lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

 namespace lldb_private {
 
 class ObjCLanguage : public Language {
   ClangHighlighter m_highlighter;
 
 public:
   class MethodName {
   public:
-    enum Type { eTypeUnspecified, eTypeClassMethod, eTypeInstanceMethod };
-
-    MethodName() : m_full(), m_class(), m_category(), m_selector() {}
-
-    MethodName(const char *name, bool strict)
-        : m_full(), m_class(), m_category(), m_selector(),
-          m_type(eTypeUnspecified), m_category_is_valid(false) {
-      SetName(name, strict);
-    }
-    MethodName(llvm::StringRef name, bool strict)
-        : m_full(), m_class(), m_category(), m_selector(),
-          m_type(eTypeUnspecified), m_category_is_valid(false) {
-      SetName(name, strict);
-    }
-
-    void Clear();
-
-    bool IsValid(bool strict) const {
-      // If "strict" is true, the name must have everything specified including
-      // the leading "+" or "-" on the method name
-      if (strict && m_type == eTypeUnspecified)
-        return false;
-      // Other than that, m_full will only be filled in if the objective C
-      // name is valid.
-      return (bool)m_full;
-    }
-
-    bool HasCategory() { return !GetCategory().IsEmpty(); }
+    /// The static factory method for creating a MethodName.
+    ///
+    /// \param[in] name
+    ///   The name of the method.
+    ///
+    /// \param[in] strict
+    ///   Control whether or not the name parser is strict about +/- in the
+    ///   front of the name.
+    ///
+    /// \return If the name did parse as a valid Objective-C method name,
+    /// returns std::nullopt. Otherwise returns a const MethodName.
+    static std::optional<const MethodName> Create(llvm::StringRef name,
+                                                  bool strict);
+
+    /// Determines if this method is a class method
+    ///
+    /// \return Returns true if the method is a class method. False otherwise.
+    bool IsClassMethod() const { return m_type == eTypeClassMethod; }
+
+    /// Determines if this method is an instance method
+    ///
+    /// \return Returns true if the method is an instance method. False
+    /// otherwise.
+    bool IsInstanceMethod() const { return m_type == eTypeInstanceMethod; }
+
+    /// Returns the full name of the method.
+    ///
+    /// This includes the class name, the category name (if applicable), and the
+    /// selector name.
+    ///
+    /// \return The name of the method in the form of a const std::string
+    /// reference.
+    const std::string &GetFullName() const { return m_full; }
+
+    /// Creates a variation of this method without the category.
+    /// If this method has no category, it returns an empty string.
+    ///
+    /// Example:
+    ///   Full name: "+[NSString(my_additions) myStringWithCString:]"
+    ///   becomes "+[NSString myStringWithCString:]"
+    ///
+    /// \return The method name without the category or an empty string if there
+    /// was no category to begin with.
+    std::string GetFullNameWithoutCategory() const;
+
+    /// Returns a reference to the class name.
+    ///
+    /// Example:
+    ///   Full name: "+[NSString(my_additions) myStringWithCString:]"
+    ///   will give you "NSString"
+    ///
+    /// \return A StringRef to the class name of this method.
+    llvm::StringRef GetClassName() const;
+
+    /// Returns a reference to the class name with the category.
+    ///
+    /// Example:
+    ///   Full name: "+[NSString(my_additions) myStringWithCString:]"
+    ///   will give you "NSString(my_additions)"
+    ///
+    /// Note: If your method has no category, this will give the same output as
+    /// `GetClassName`.
+    ///
+    /// \return A StringRef to the class name (including the category) of this
+    /// method.
+    llvm::StringRef GetClassNameWithCategory() const;
+
+    /// Returns a reference to the category name.
+    ///
+    /// Example:
+    ///   Full name: "+[NSString(my_additions) myStringWithCString:]"
+    ///   will give you "my_additions"
+    /// \return A StringRef to the category name of this method.
+    llvm::StringRef GetCategory() const;
+
+    /// Returns a reference to the selector name.
+    ///
+    /// Example:
+    ///   Full name: "+[NSString(my_additions) myStringWithCString:]"
+    ///   will give you "myStringWithCString:"
+    /// \return A StringRef to the selector of this method.
+    llvm::StringRef GetSelector() const;
 
-    Type GetType() const { return m_type; }
-
-    ConstString GetFullName() const { return m_full; }
-
-    ConstString GetFullNameWithoutCategory(bool empty_if_no_category);
-
-    bool SetName(const char *name, bool strict);
-    bool SetName(llvm::StringRef name, bool strict);
-
-    ConstString GetClassName();
-
-    ConstString GetClassNameWithCategory();
-
-    ConstString GetCategory();
+  protected:
+    enum Type { eTypeUnspecified, eTypeClassMethod, eTypeInstanceMethod };
 
-    ConstString GetSelector();
+    MethodName(llvm::StringRef name, Type type)
+        : m_full(name.str()), m_type(type) {}
 
-  protected:
-    ConstString
-        m_full; // Full name:   "+[NSString(my_additions) myStringWithCString:]"
-    ConstString m_class; // Class name:  "NSString"
-    ConstString
-        m_class_category;   // Class with category: "NSString(my_additions)"
-    ConstString m_category; // Category:    "my_additions"
-    ConstString m_selector; // Selector:    "myStringWithCString:"
-    Type m_type = eTypeUnspecified;
-    bool m_category_is_valid = false;
+    const std::string m_full;
+    Type m_type;
   };
 
   ObjCLanguage() = default;
 
   ~ObjCLanguage() override = default;
 
   lldb::LanguageType GetLanguageType() const override {
     return lldb::eLanguageTypeObjC;

lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp

Language *ObjCLanguage::CreateInstance(lldb::LanguageType language) {
   switch (language) {
   case lldb::eLanguageTypeObjC:
     return new ObjCLanguage();
   default:
     return nullptr;
   }
 }
 
-void ObjCLanguage::MethodName::Clear() {
-  m_full.Clear();
-  m_class.Clear();
-  m_category.Clear();
-  m_selector.Clear();
-  m_type = eTypeUnspecified;
-  m_category_is_valid = false;
-}
-
-bool ObjCLanguage::MethodName::SetName(llvm::StringRef name, bool strict) {
-  Clear();
+std::optional<const ObjCLanguage::MethodName>
+ObjCLanguage::MethodName::Create(llvm::StringRef name, bool strict) {
   if (name.empty())
-    return IsValid(strict);
-
-  // If "strict" is true. then the method must be specified with a '+' or '-'
-  // at the beginning. If "strict" is false, then the '+' or '-' can be omitted
-  bool valid_prefix = false;
-
-  if (name.size() > 1 && (name[0] == '+' || name[0] == '-')) {
-    valid_prefix = name[1] == '[';
-    if (name[0] == '+')
-      m_type = eTypeClassMethod;
-    else
-      m_type = eTypeInstanceMethod;
-  } else if (!strict) {
-    // "strict" is false, the name just needs to start with '['
-    valid_prefix = name[0] == '[';
-  }
-
-  if (valid_prefix) {
-    int name_len = name.size();
-    // Objective-C methods must have at least:
-    //      "-[" or "+[" prefix
-    //      One character for a class name
-    //      One character for the space between the class name
-    //      One character for the method name
-    //      "]" suffix
-    if (name_len >= (5 + (strict ? 1 : 0)) && name.back() == ']') {
-      m_full.SetString(name);
-    }
-  }
-  return IsValid(strict);
-}
-
-bool ObjCLanguage::MethodName::SetName(const char *name, bool strict) {
-  return SetName(llvm::StringRef(name), strict);
-}
-
-ConstString ObjCLanguage::MethodName::GetClassName() {
-  if (!m_class) {
-    if (IsValid(false)) {
-      const char *full = m_full.GetCString();
-      const char *class_start = (full[0] == '[' ? full + 1 : full + 2);
-      const char *paren_pos = strchr(class_start, '(');
-      if (paren_pos) {
-        m_class.SetCStringWithLength(class_start, paren_pos - class_start);
-      } else {
-        // No '(' was found in the full name, we can definitively say that our
-        // category was valid (and empty).
-        m_category_is_valid = true;
-        const char *space_pos = strchr(full, ' ');
-        if (space_pos) {
-          m_class.SetCStringWithLength(class_start, space_pos - class_start);
-          if (!m_class_category) {
-            // No category in name, so we can also fill in the m_class_category
-            m_class_category = m_class;
-          }
-        }
-      }
-    }
-  }
-  return m_class;
-}
-
-ConstString ObjCLanguage::MethodName::GetClassNameWithCategory() {
-  if (!m_class_category) {
-    if (IsValid(false)) {
-      const char *full = m_full.GetCString();
-      const char *class_start = (full[0] == '[' ? full + 1 : full + 2);
-      const char *space_pos = strchr(full, ' ');
-      if (space_pos) {
-        m_class_category.SetCStringWithLength(class_start,
-                                              space_pos - class_start);
-        // If m_class hasn't been filled in and the class with category doesn't
-        // contain a '(', then we can also fill in the m_class
-        if (!m_class && strchr(m_class_category.GetCString(), '(') == nullptr) {
-          m_class = m_class_category;
-          // No '(' was found in the full name, we can definitively say that
-          // our category was valid (and empty).
-          m_category_is_valid = true;
-        }
-      }
-    }
-  }
-  return m_class_category;
-}
+    return std::nullopt;
 
-ConstString ObjCLanguage::MethodName::GetSelector() {
-  if (!m_selector) {
-    if (IsValid(false)) {
-      const char *full = m_full.GetCString();
-      const char *space_pos = strchr(full, ' ');
-      if (space_pos) {
-        ++space_pos; // skip the space
-        m_selector.SetCStringWithLength(space_pos, m_full.GetLength() -
-                                                       (space_pos - full) - 1);
-      }
-    }
-  }
-  return m_selector;
-}
-
-ConstString ObjCLanguage::MethodName::GetCategory() {
-  if (!m_category_is_valid && !m_category) {
-    if (IsValid(false)) {
-      m_category_is_valid = true;
-      const char *full = m_full.GetCString();
-      const char *class_start = (full[0] == '[' ? full + 1 : full + 2);
-      const char *open_paren_pos = strchr(class_start, '(');
-      if (open_paren_pos) {
-        ++open_paren_pos; // Skip the open paren
-        const char *close_paren_pos = strchr(open_paren_pos, ')');
-        if (close_paren_pos)
-          m_category.SetCStringWithLength(open_paren_pos,
-                                          close_paren_pos - open_paren_pos);
-      }
-    }
-  }
-  return m_category;
-}
+  // Objective-C method minimum requirements:
+  //  - If `strict` is true, must start with '-' or '+' (1 char)
+  //  - Must be followed by '[' (1 char)
+  //  - Must have at least one character for class name (1 char)
+  //  - Must have a space between class name and method name (1 char)
+  //  - Must have at least one character for  method name (1 char)
+  //  - Must be end with ']' (1 char)
+  //  This means that the minimum size is 5 characters (6 if `strict`)
+  //  e.g. [a a] (-[a a] or +[a a] if `strict`)
+
+  // We can check length and ending invariants first
+  if (name.size() < (5 + (strict ? 1 : 0)) || name.back() != ']')
+    return std::nullopt;
+
+  // Figure out type
+  Type type = eTypeUnspecified;
+  if (name.startswith("+["))
+    type = eTypeClassMethod;
+  else if (name.startswith("-["))
+    type = eTypeInstanceMethod;
+
+  // If there's no type and it's strict, this is invalid
+  if (strict && type == eTypeUnspecified)
+    return std::nullopt;
+
+  // If not strict and type unspecified, make sure we start with '['
+  if (type == eTypeUnspecified && name.front() != '[')
+    return std::nullopt;
+
+  // If we've gotten here, we're confident that this looks enough like an
+  // Objective-C method to treat it like one.
+  ObjCLanguage::MethodName method_name(name, type);
+  return method_name;
+}
+
+llvm::StringRef ObjCLanguage::MethodName::GetClassName() const {
+  llvm::StringRef full = m_full;
+  const size_t class_start_pos = (full.front() == '[' ? 1 : 2);
+  const size_t paren_pos = full.find('(', class_start_pos);
+  // If there's a category we want to stop there
+  if (paren_pos != llvm::StringRef::npos)
+    return full.substr(class_start_pos, paren_pos - class_start_pos);
+
+  // Otherwise we find the space separating the class and method
+  const size_t space_pos = full.find(' ', class_start_pos);
+  return full.substr(class_start_pos, space_pos - class_start_pos);
+}
+
+llvm::StringRef ObjCLanguage::MethodName::GetClassNameWithCategory() const {
+  llvm::StringRef full = m_full;
+  const size_t class_start_pos = (full.front() == '[' ? 1 : 2);
+  const size_t space_pos = full.find(' ', class_start_pos);
+  return full.substr(class_start_pos, space_pos - class_start_pos);
+}
+
+llvm::StringRef ObjCLanguage::MethodName::GetSelector() const {
+  llvm::StringRef full = m_full;
+  const size_t space_pos = full.find(' ');
+  if (space_pos == llvm::StringRef::npos)
+    return llvm::StringRef();
+  const size_t closing_bracket = full.find(']', space_pos);
+  return full.substr(space_pos + 1, closing_bracket - space_pos - 1);
+}
+
+llvm::StringRef ObjCLanguage::MethodName::GetCategory() const {
+  llvm::StringRef full = m_full;
+  const size_t open_paren_pos = full.find('(');
+  const size_t close_paren_pos = full.find(')');
+
+  if (open_paren_pos == llvm::StringRef::npos ||
+      close_paren_pos == llvm::StringRef::npos)
+    return llvm::StringRef();
+
+  return full.substr(open_paren_pos + 1,
+                     close_paren_pos - (open_paren_pos + 1));
+}
+
+std::string ObjCLanguage::MethodName::GetFullNameWithoutCategory() const {
+  llvm::StringRef full = m_full;
+  const size_t open_paren_pos = full.find('(');
+  const size_t close_paren_pos = full.find(')');
+  if (open_paren_pos == llvm::StringRef::npos ||
+      close_paren_pos == llvm::StringRef::npos)
+    return std::string();
+
+  llvm::StringRef class_name = GetClassName();
+  llvm::StringRef selector_name = GetSelector();
+  std::string name_sans_category;
 
-ConstString ObjCLanguage::MethodName::GetFullNameWithoutCategory(
-    bool empty_if_no_category) {
-  if (IsValid(false)) {
-    if (HasCategory()) {
-      StreamString strm;
-      if (m_type == eTypeClassMethod)
-        strm.PutChar('+');
-      else if (m_type == eTypeInstanceMethod)
-        strm.PutChar('-');
-      strm.Printf("[%s %s]", GetClassName().GetCString(),
-                  GetSelector().GetCString());
-      return ConstString(strm.GetString());
-    }
+    name_sans_category += '+';
+    name_sans_category += '-';
 
-    if (!empty_if_no_category) {
-      // Just return the full name since it doesn't have a category
-      return GetFullName();
-    }
-  }
-  return ConstString();
+  name_sans_category += '[';
+  name_sans_category.append(class_name.data(), class_name.size());
+  name_sans_category += ' ';
+  name_sans_category.append(selector_name.data(), selector_name.size());
+  name_sans_category += ']';
+
+  return name_sans_category;
 }
 
 std::vector<Language::MethodNameVariant>
 ObjCLanguage::GetMethodNameVariants(ConstString method_name) const {
   std::vector<Language::MethodNameVariant> variant_names;
-  ObjCLanguage::MethodName objc_method(method_name.GetCString(), false);
-  if (!objc_method.IsValid(false)) {
+  std::optional<const ObjCLanguage::MethodName> objc_method =
+      ObjCLanguage::MethodName::Create(method_name.GetStringRef(), false);
+  if (!objc_method)
     return variant_names;
-  }
 
-  variant_names.emplace_back(objc_method.GetSelector(),
+  variant_names.emplace_back(ConstString(objc_method->GetSelector()),
                              lldb::eFunctionNameTypeSelector);
 
-  const bool is_class_method =
-      objc_method.GetType() == MethodName::eTypeClassMethod;
-  const bool is_instance_method =
-      objc_method.GetType() == MethodName::eTypeInstanceMethod;
-  ConstString name_sans_category =
-      objc_method.GetFullNameWithoutCategory(/*empty_if_no_category*/ true);
-
-  if (is_class_method || is_instance_method) {
-    if (name_sans_category)
-      variant_names.emplace_back(name_sans_category,
+  const std::string name_sans_category =
+      objc_method->GetFullNameWithoutCategory();
+
+  if (objc_method->IsClassMethod() || objc_method->IsInstanceMethod()) {
+    if (!name_sans_category.empty())
+      variant_names.emplace_back(ConstString(name_sans_category.c_str()),
                                  lldb::eFunctionNameTypeFull);
   } else {
     StreamString strm;
 
-    strm.Printf("+%s", objc_method.GetFullName().GetCString());
+    strm.Printf("+%s", objc_method->GetFullName().c_str());
     variant_names.emplace_back(ConstString(strm.GetString()),
                                lldb::eFunctionNameTypeFull);
     strm.Clear();
 
-    strm.Printf("-%s", objc_method.GetFullName().GetCString());
+    strm.Printf("-%s", objc_method->GetFullName().c_str());
     variant_names.emplace_back(ConstString(strm.GetString()),
                                lldb::eFunctionNameTypeFull);
     strm.Clear();
 
-    if (name_sans_category) {
-      strm.Printf("+%s", name_sans_category.GetCString());
+    if (!name_sans_category.empty()) {
+      strm.Printf("+%s", name_sans_category.c_str());
       variant_names.emplace_back(ConstString(strm.GetString()),
                                  lldb::eFunctionNameTypeFull);
       strm.Clear();
 
-      strm.Printf("-%s", name_sans_category.GetCString());
+      strm.Printf("-%s", name_sans_category.c_str());
       variant_names.emplace_back(ConstString(strm.GetString()),
                                  lldb::eFunctionNameTypeFull);
     }
   }
 
   return variant_names;
 }
 
bool Find_Impl(ExecutionContextScope *exe_scope, const char *key,
         }
       }
 
       return result;
     }
 
     friend class lldb_private::ObjCLanguage;
   };
-  
   class ObjCDebugInfoScavenger : public Language::ImageListTypeScavenger {
   public:
     CompilerType AdjustForInclusion(CompilerType &candidate) override {
       LanguageType lang_type(candidate.GetMinimumLanguage());
       if (!Language::LanguageIsObjC(lang_type))
         return CompilerType();
       if (candidate.IsTypedefType())
         return candidate.GetTypedefedType();

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

TypeSP DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
   CompilerType clang_type =
       m_ast.CreateFunctionType(return_clang_type, function_param_types.data(),
                                function_param_types.size(), is_variadic,
                                type_quals, calling_convention, attrs.ref_qual);
 
   if (attrs.name) {
     bool type_handled = false;
     if (tag == DW_TAG_subprogram || tag == DW_TAG_inlined_subroutine) {
-      ObjCLanguage::MethodName objc_method(attrs.name.GetStringRef(), true);
-      if (objc_method.IsValid(true)) {
+      std::optional<const ObjCLanguage::MethodName> objc_method =
+          ObjCLanguage::MethodName::Create(attrs.name.GetStringRef(), true);
+      if (objc_method) {
         CompilerType class_opaque_type;
-        ConstString class_name(objc_method.GetClassName());
+        ConstString class_name(objc_method->GetClassName());
         if (class_name) {
           TypeSP complete_objc_class_type_sp(
               dwarf->FindCompleteObjCDefinitionTypeForDIE(DWARFDIE(),
                                                           class_name, false));
 
           if (complete_objc_class_type_sp) {
             CompilerType type_clang_forward_type =
                 complete_objc_class_type_sp->GetForwardCompilerType();
PropertyAttributes::PropertyAttributes(const DWARFDIE &die) {
 
   if (!prop_name)
     return;
   ConstString fixed_setter;
 
   // Check if the property getter/setter were provided as full names.
   // We want basenames, so we extract them.
   if (prop_getter_name && prop_getter_name[0] == '-') {
-    ObjCLanguage::MethodName prop_getter_method(prop_getter_name, true);
-    prop_getter_name = prop_getter_method.GetSelector().GetCString();
+    std::optional<const ObjCLanguage::MethodName> prop_getter_method =
+        ObjCLanguage::MethodName::Create(prop_getter_name, true);
+    if (prop_getter_method)
+      prop_getter_name =
+          ConstString(prop_getter_method->GetSelector()).GetCString();
   }
 
   if (prop_setter_name && prop_setter_name[0] == '-') {
-    ObjCLanguage::MethodName prop_setter_method(prop_setter_name, true);
-    prop_setter_name = prop_setter_method.GetSelector().GetCString();
+    std::optional<const ObjCLanguage::MethodName> prop_setter_method =
+        ObjCLanguage::MethodName::Create(prop_setter_name, true);
+    if (prop_setter_method)
+      prop_setter_name =
+          ConstString(prop_setter_method->GetSelector()).GetCString();
   }
 
   // If the names haven't been provided, they need to be filled in.
   if (!prop_getter_name)
     prop_getter_name = prop_name;
   if (!prop_setter_name && prop_name[0] &&
       !(prop_attributes & DW_APPLE_PROPERTY_readonly)) {
     StreamString ss;

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

for (const DWARFDebugInfoEntry &die : unit.dies()) {
     switch (tag) {
     case DW_TAG_inlined_subroutine:
     case DW_TAG_subprogram:
       if (has_address) {
         if (name) {
           bool is_objc_method = false;
           if (cu_language == eLanguageTypeObjC ||
               cu_language == eLanguageTypeObjC_plus_plus) {
-            ObjCLanguage::MethodName objc_method(name, true);
-            if (objc_method.IsValid(true)) {
+            std::optional<const ObjCLanguage::MethodName> objc_method =
+                ObjCLanguage::MethodName::Create(name, true);
+            if (objc_method) {
               is_objc_method = true;
               ConstString class_name_with_category(
-                  objc_method.GetClassNameWithCategory());
-              ConstString objc_selector_name(objc_method.GetSelector());
+                  objc_method->GetClassNameWithCategory());
+              ConstString objc_selector_name(objc_method->GetSelector());
               ConstString objc_fullname_no_category_name(
-                  objc_method.GetFullNameWithoutCategory(true));
-              ConstString class_name_no_category(objc_method.GetClassName());
+                  objc_method->GetFullNameWithoutCategory().c_str());
+              ConstString class_name_no_category(objc_method->GetClassName());
               set.function_fullnames.Insert(ConstString(name), ref);
               if (class_name_with_category)
                 set.objc_class_selectors.Insert(class_name_with_category, ref);
               if (class_name_no_category &&
                   class_name_no_category != class_name_with_category)
                 set.objc_class_selectors.Insert(class_name_no_category, ref);
               if (objc_selector_name)
                 set.function_selectors.Insert(objc_selector_name, ref);

lldb/unittests/Language/ObjC/ObjCLanguageTest.cpp

TEST(ObjCLanguage, MethodNameParsing) {
   TestCase lax_cases[] = {
       {"[MyClass mySelector:]", "", "MyClass", "MyClass", "", "mySelector:"},
       {"[MyClass(my_category) mySelector:]", "[MyClass mySelector:]", "MyClass",
        "MyClass(my_category)", "my_category", "mySelector:"},
   };
 
   // First, be strict
   for (const auto &test : strict_cases) {
-    ObjCLanguage::MethodName method(test.input, /*strict = */ true);
-    EXPECT_TRUE(method.IsValid(/*strict = */ true));
-    EXPECT_EQ(
-        test.full_name_sans_category,
-        method.GetFullNameWithoutCategory(/*empty_if_no_category = */ true)
-            .GetStringRef());
-    EXPECT_EQ(test.class_name, method.GetClassName().GetStringRef());
+    std::optional<const ObjCLanguage::MethodName> method =
+        ObjCLanguage::MethodName::Create(test.input, /*strict = */ true);
+    EXPECT_TRUE(method.has_value());
+    EXPECT_EQ(test.full_name_sans_category,
+              method->GetFullNameWithoutCategory());
+    EXPECT_EQ(test.class_name, method->GetClassName());
     EXPECT_EQ(test.class_name_with_category,
-              method.GetClassNameWithCategory().GetStringRef());
-    EXPECT_EQ(test.category, method.GetCategory().GetStringRef());
-    EXPECT_EQ(test.selector, method.GetSelector().GetStringRef());
+              method->GetClassNameWithCategory());
+    EXPECT_EQ(test.category, method->GetCategory());
+    EXPECT_EQ(test.selector, method->GetSelector());
   }
 
   // We should make sure strict parsing does not accept lax cases
   for (const auto &test : lax_cases) {
-    ObjCLanguage::MethodName method(test.input, /*strict = */ true);
-    EXPECT_FALSE(method.IsValid(/*strict = */ true));
+    std::optional<const ObjCLanguage::MethodName> method =
+        ObjCLanguage::MethodName::Create(test.input, /*strict = */ true);
+    EXPECT_FALSE(method.has_value());
   }
 
   // All strict cases should work when not lax
   for (const auto &test : strict_cases) {
-    ObjCLanguage::MethodName method(test.input, /*strict = */ false);
-    EXPECT_TRUE(method.IsValid(/*strict = */ false));
-    EXPECT_EQ(
-        test.full_name_sans_category,
-        method.GetFullNameWithoutCategory(/*empty_if_no_category = */ true)
-            .GetStringRef());
-    EXPECT_EQ(test.class_name, method.GetClassName().GetStringRef());
+    std::optional<const ObjCLanguage::MethodName> method =
+        ObjCLanguage::MethodName::Create(test.input, /*strict = */ false);
+    EXPECT_TRUE(method.has_value());
+    EXPECT_EQ(test.full_name_sans_category,
+              method->GetFullNameWithoutCategory());
+    EXPECT_EQ(test.class_name, method->GetClassName());
     EXPECT_EQ(test.class_name_with_category,
-              method.GetClassNameWithCategory().GetStringRef());
-    EXPECT_EQ(test.category, method.GetCategory().GetStringRef());
-    EXPECT_EQ(test.selector, method.GetSelector().GetStringRef());
+              method->GetClassNameWithCategory());
+    EXPECT_EQ(test.category, method->GetCategory());
+    EXPECT_EQ(test.selector, method->GetSelector());
   }
 
   // Make sure non-strict parsing works
   for (const auto &test : lax_cases) {
-    ObjCLanguage::MethodName method(test.input, /*strict = */ false);
-    EXPECT_TRUE(method.IsValid(/*strict = */ false));
-    EXPECT_EQ(
-        test.full_name_sans_category,
-        method.GetFullNameWithoutCategory(/*empty_if_no_category = */ true)
-            .GetStringRef());
-    EXPECT_EQ(test.class_name, method.GetClassName().GetStringRef());
+    std::optional<const ObjCLanguage::MethodName> method =
+        ObjCLanguage::MethodName::Create(test.input, /*strict = */ false);
+    EXPECT_TRUE(method.has_value());
+    EXPECT_EQ(test.full_name_sans_category,
+              method->GetFullNameWithoutCategory());
+    EXPECT_EQ(test.class_name, method->GetClassName());
     EXPECT_EQ(test.class_name_with_category,
-              method.GetClassNameWithCategory().GetStringRef());
-    EXPECT_EQ(test.category, method.GetCategory().GetStringRef());
-    EXPECT_EQ(test.selector, method.GetSelector().GetStringRef());
+              method->GetClassNameWithCategory());
+    EXPECT_EQ(test.category, method->GetCategory());
+    EXPECT_EQ(test.selector, method->GetSelector());
   }
 }
 
 TEST(CPlusPlusLanguage, InvalidMethodNameParsing) {
   // Tests that we correctly reject malformed function names
 
   llvm::StringRef test_cases[] = {"+[Uh oh!",
                                   "-[Definitely not...",
                                   "[Nice try ] :)",
                                   "+MaybeIfYouSquintYourEyes]",
                                   "?[Tricky]",
                                   "+[]",
                                   "-[]",
                                   "[]"};
 
   for (const auto &name : test_cases) {
-    ObjCLanguage::MethodName strict_method(name, /*strict = */ true);
-    EXPECT_FALSE(strict_method.IsValid(true));
-
-    ObjCLanguage::MethodName lax_method(name, /*strict = */ false);
-    EXPECT_FALSE(lax_method.IsValid(true));
+    std::optional<const ObjCLanguage::MethodName> strict_method =
+        ObjCLanguage::MethodName::Create(name, /*strict = */ false);
+    EXPECT_FALSE(strict_method.has_value());
+
+    std::optional<const ObjCLanguage::MethodName> lax_method =
+        ObjCLanguage::MethodName::Create(name, /*strict = */ false);
+    EXPECT_FALSE(lax_method.has_value());
   }
 }