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