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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
