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

[via lldb-commits]

cmtice 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)

I've tried this out: I updated the FormatDiagnostics function to use DiagnosticDetail and RenderDiagnosticDetails.  This does give nicely formatted error messages similar to the full expression parser (the old error messages were closer to what the current frame variable implementation does). But I'm not really sure this is worth it? It looks to me like the diagnostics infrastructure was really designed to handle multiple errors.  But a key design point in DIL is to fail fast, i.e. as soon as we encounter a definite error. So in our case we will only ever be processing a single error for any given input (the first one), no matter how many errors the input actually contains.

I've updated the code: The new stuff, using DiagnosticDetail, etc. is in NewFormatDiagnostics (in DILParser.cpp).  I've left the old code, just for comparison, in OldFormatDiagnostics, just below. PTAL a let me know which you think makes more sense for us (and also let me know if I've done something not quite right in the new code, if we decide to go that route).


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