[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

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 up in the ConstString pools for symbol names 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.



================
Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:37
+    ///
+    /// \return If the name did parse as a valid Objective-C method name,
+    /// returns std::nullopt. Otherwise returns a const MethodName.
----------------



================
Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:100
+    ///   will give you "my_additions"
+    /// \return A StringRef to the category name of this method.
+    llvm::StringRef GetCategory() const;
----------------
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...


================
Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:117
 
-  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;
----------------
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... 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149914/new/

https://reviews.llvm.org/D149914