[Lldb-commits] [PATCH] D63240: [Core] Generalize ValueObject::IsRuntimeSupportValue

[Alex Langford via Phabricator via lldb-commits]

xiaobai marked 3 inline comments as done.
xiaobai added inline comments.


================
Comment at: include/lldb/Target/LanguageRuntime.h:160-162
+  /// Identify whether a name is a runtime value that should not be hidden by
+  /// from the user interface.
+  virtual bool IsWhitelistedRuntimeValue(ConstString name) { return false; }
----------------
labath wrote:
> Could we make this a non-public extension point, and make it so that it is automatically invoked from `IsRuntimeSupportValue` ?
> 
> That would simplify the code on the caller side..
See my comment on ValueObject::IsRuntimeSupportValue to see why I think this isn't going to be enough.


================
Comment at: source/Core/ValueObject.cpp:1702-1724
+      return runtime->IsRuntimeSupportValue(*this) &&
+             !runtime->IsWhitelistedRuntimeValue(GetName());
+
+    // It's possible we have more than one language involved here. For example,
+    // in ObjC `_cmd` is a whitelisted runtime variable name, but
+    // GetObjectRuntimeLanguage will say it's a C variable since it's just a
+    // cstring.
----------------
clayborg wrote:
> Seems like this should still just make one call. The two calls to IsRuntimeSupportValue followed by IsWhitelistedRuntimeValue seems like a bunch of hard to read code. We should make just one call to each runtime and do all the work needed from within there. So I vote to revert this change and do the work in each IsRuntimeSupportValue method. See additional comments in CPPLanguageRuntime::IsRuntimeSupportValue() that was removed. 
I'm not convinced that doing all the work in `LanguageRuntime::IsRuntimeSupportValue` is the right thing to do. The interface doesn't make a distinction between "Isn't a runtime support value" and "Is a whitelisted runtime support value".
There's not a good way to know. The implementation I have here keeps track of whether or not any language runtime considers this a runtime support value. If a runtime considers it a runtime support value and it's a whitelisted one, you immediately return false. If you have just `IsRuntimeSupportValue` and one runtime says "this is a runtime support value" but the other says "this isn't a runtime support value", how do you know if the second runtime is trying to say it's a whitelisted value?

However, getting rid of the initial GetLanguageRuntime+check in favor of just having one big loop seems like it'll be better so I'll change that.


================
Comment at: source/Target/CPPLanguageRuntime.cpp:59
-    return false;
-  return !ObjCLanguageRuntime::IsWhitelistedRuntimeValue(name);
 }
----------------
clayborg wrote:
> Seems like we should restore this function and just get the ObjC language runtime from the process, and if it exists, call IsWhitelistedRuntimeValue on it?
> 
> ```
> auto objc_runtime = ObjCLanguageRuntime::Get(m_process);
> if (objc_runtime)
>   return objc_runtime->IsWhitelistedRuntimeValue(name);
> return false;
> ```
Indeed, I did not preserve the semantics here because they are technically wrong. It means that, regardless of whether or not you have an ObjC runtime loaded, you still make sure it's being handled.

Greg's answer is the correct answer while trying to preserve the intention. I think that this is unnecessary though, because `ValueObject::IsRuntimeSupportValue` after this patch should check every available runtime. If you care about ObjC++, the ObjC language runtime should handle the ObjC whitelisted runtime values.


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

https://reviews.llvm.org/D63240