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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D149914#4331048 <https://reviews.llvm.org/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.



================
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.
----------------
jingham wrote:
> 
Good catch, thanks!


================
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;
----------------
jingham wrote:
> 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...
`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.




================
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;
----------------
jingham wrote:
> 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... 
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 `ConstString`s 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149914