[Lldb-commits] [PATCH] D64159: [Core] Generalize ValueObject::MaybeCalculateCompleteType

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

The main issue I have with this is the name of the function we are adding to LanguageRuntime. See inlined comments.



================
Comment at: include/lldb/Target/ObjCLanguageRuntime.h:253
 
+  CompilerType CalculateCompleteType(CompilerType base_type) override;
+
----------------
Is this named correctly? Maybe this should be named "CompilerType GetRuntimeType(CompilerType base_type) override;"? 

What this function does is gets the real definition from the objective C runtime at the moment. This name would better reflect what is going on and would be something we might ask of a runtime.



================
Comment at: source/Target/ObjCLanguageRuntime.cpp:403
+CompilerType
+ObjCLanguageRuntime::CalculateCompleteType(CompilerType base_type) {
+  CompilerType type_to_return;
----------------
So a main question for ObjC here: do we always want to show the runtime type? Should we not check if the class inside of "base_type" is the one true definition and skip grabbing the runtime type here and return {}?


================
Comment at: source/Target/ObjCLanguageRuntime.cpp:404
+ObjCLanguageRuntime::CalculateCompleteType(CompilerType base_type) {
+  CompilerType type_to_return;
+
----------------
JDevlieghere wrote:
> I would move this down.
Remove this variable and just return {} everywhere


================
Comment at: source/Target/ObjCLanguageRuntime.cpp:430-433
+    if (is_pointer_type)
+      type_to_return = complete_class.GetPointerType();
+    else
+      type_to_return = complete_class;
----------------
```
if (is_pointer_type)
  return complete_class.GetPointerType();
else
  return complete_class;
```


================
Comment at: source/Target/ObjCLanguageRuntime.cpp:436
+
+  return type_to_return;
+}
----------------
```
return {};
```


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

https://reviews.llvm.org/D64159