[Lldb-commits] [PATCH] D99315: [lldb] Support lazily named classes in the Objective-C classes

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 4 inline comments as done.
JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h:305
+  /// named classes.
+  class GetClassInfo {
+  public:
----------------
aprantl wrote:
> GetClassInfo is an odd name for a class. It sounds more like a function. I'm also not suggesting ClassInfoFactory :-p
I went with `DynamicClassInfoExtractor`. I'm planning on moving the corresponding 3 instance variables for the shared cache into a `SharedCacheClassInfoExtractor` in a follow up patch. 


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h:309
+
+    UtilityFunction *GetClassInfoUtilityFunction(ExecutionContext &exe_ctx,
+                                                 Helper helper);
----------------
aprantl wrote:
> Can this return a nullptr? If not, then this should return a reference.
Yes, if we fail to create the utility function, we'll log the error and return a nullptr. 


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h:317
+    /// gdb_objc_realized_classes otherwise.
+    static Helper ComputeHelper(AppleObjCRuntimeV2 &runtime);
+
----------------
aprantl wrote:
> Why does this need to be public? I thought the idea of the helper class was to hide the actual implementation?
We need to make sure we stick to one way of updating the class info. At some point I want to move most of `UpdateISAToDescriptorMapDynamic` into this class, at which this no longer needs to be private. For this patch I tried to keep unrelated refactorings to a minimum. 


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h:331
+    /// Utility function to read class info using objc_copyRealizedClassList.
+    std::unique_ptr<UtilityFunction> m_get_class_info2_code;
+    lldb::addr_t m_get_class_info2_args = LLDB_INVALID_ADDRESS;
----------------
aprantl wrote:
> Any better naming ideas?
Not proud of this, but I found that `gdb_objc_realized_classes` and `objc_copyRealizedClassList` are looked so similar that it was choosing between the pest and the cholera. 


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

https://reviews.llvm.org/D99315