[Lldb-commits] [lldb] [LLDB] Add DIL code for handling plain variable names. (PR #120971)

[Pavel Labath via lldb-commits]

labath wrote:

> > Didn't quite finish, but this is what I have so far. I am wondering about the usefulness of the `IdentifierInfo`. It started out its existence in a completely different world than what we have now -- where the operations were mainly defined on types instead of values. Does it bring us anything in the new setup or could we replace it by passing ValueObjects directly?
> 
> When doing straight variable name lookup, I agree that there's not much added benefit to using the IdentifierNodes, but they're needed more (at the moment) when we're looking up field member names. Unless you strongly object, I'd like to keep them for now, and maybe revisit their usefulness when looking at the PR that handles field/member name resolution.

I'm somewhat tempted to say we should do the opposite (remove it now, and reintroduce it later if it is needed), but it would be nice to avoid doing that work if it's going to be reintroduced anyway. Can you explain why you think it is necessary for child member resolution (i.e., what does it tell you that cannot be obtained from a ValueObject representing the child member)?

> 
> > The error handling isn't quite as I imagined, but that's mostly a cosmetic thing we can handle when I get back.
> > And I think Adrian's question of whether this should be returning structured error messages (diagnostics) isn't quite resolved (I think we should do that).
> 
> I though you had said you weren't sure that using the DiagnosticsManager here made sense? We do have somewhat formatted errors:
> 
> (lldb) v (1.0+c)+(1.0+d) error: expr:1:5: invalid operands to binary expression ('double' and 'mynumber') (1.0+c)+(1.0+d) ^ (lldb)
> 
> But if you want me to try to use the DiagnosticManager for DIL errors, I will...

I said I'm not sure about using the DiagnosticManager, and that part is still true. However, that's a different question from returning structured (not just formatted) errors (i.e., instances of the [DiagnosticError](https://github.com/llvm/llvm-project/blob/1ff10fa82fff83bb2f0a5c1ffde6203b52bc9619/lldb/include/lldb/Utility/DiagnosticsRendering.h#L67) class), which I think we should do. In the "real" expression evaluator the DiagnosticManager creates the `DiagnosticError`s (actually a subclass of them) from the compiler diagnostics, but there's no reason why you cannot create one manually (somewhere near the BailOut function). The question is whether it's simpler to do that or to reuse the functionality in the DiagnosticManager (I'm sort of leaning towards not using it)

https://github.com/llvm/llvm-project/pull/120971