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