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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
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.
----------------
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. 


================
Comment at: source/Target/CPPLanguageRuntime.cpp:59
-    return false;
-  return !ObjCLanguageRuntime::IsWhitelistedRuntimeValue(name);
 }
----------------
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;
```


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

https://reviews.llvm.org/D63240