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

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

In D63240#1559994 <https://reviews.llvm.org/D63240#1559994>, @jingham wrote:

> To be more precise, "frame" is the wrong word to use, Variables have "scopes"...  All Variables have a scope, though not every scope is contained in a Function.
>
> But just to back up a little bit.
>
> We're getting into problems, it seems to me, because we're asking ValueObjects questions that should be asked of their containing scope.  For instance, if you are in a Function, and want to list the locals and arguments, you want to know what the rules for the Function are for which artificial variables should and should not be printed, not any of its value objects.
>
> For instance, ObjC could decide that it wants to reserve the word "this" in methods of ObjC classes in ObjC++ files, and use it as a should-be-hidden artificial variable that will always store a pointer to C++ object for some evil purposes.  Asking the runtime language of this artificial "this" variable whether the word "this" is runtime whitelisted seems like the wrong thing to do.  After all, the Variable has a CompilerType that's clearly a C++ object, so you might reasonably expect its runtime language to be C++.  But then you would get the wrong answer.  You really need to ask the Function "what are your rules for hiding artificial variables".
>
> Getting to the right actor to ask about "which artificial variables to show" was the reason for suggesting that the runtime language of a Variable should be that of its containing scope.  But then you get into the weird situation where a C++ Variable is claiming a runtime language of ObjC++, which seems a little odd.  Instead, it makes more sense to get the RuntimeLanguage of the current frame, and then iterate over the variables and ask the frame's runtime whether they should be shown or not.
>
> I'm not sure what to do about global variables.  Reasoning by analogy, that's something that the language of the CompileUnit which contains the global variables should answer.


If I understood you correctly, your conclusion is that `ValueObject::IsRuntimeSupportValue` shouldn't really exist and that you should be asking a frame or a compilation unit if a value should be displayed to the user. I think that this is reasonable. That being said, I think that this idea will have some interesting challenges as well. One problem that I'm aware of at the moment is that SBValue has `IsRuntimeSupportValue`. If I remember correctly, removing from the SBAPI is generally not permitted (something I disagree with in general, but I digress).

In the short term, I'd like to get this patch (or some variation of it) into LLDB so that we can remove ValueObject's dependence on ObjC (after this there's one reference left but I have a good idea of how to remove it). Would you mind if I committed this, or do you feel strongly that we should first refactor further?


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

https://reviews.llvm.org/D63240