[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

Fri Jul 26 11:17:28 PDT 2019

xiaobai added a comment.

I'm going to update this diff with what I changed to give y'all a better idea of what has been changed. I shoulda done that to begin with... :P

In D65122#1602145 <https://reviews.llvm.org/D65122#1602145>, @labath wrote:

> In D65122#1602067 <https://reviews.llvm.org/D65122#1602067>, @JDevlieghere wrote:
>
> > In D65122#1602025 <https://reviews.llvm.org/D65122#1602025>, @xiaobai wrote:
> >
> > > After going through this and modifying this patch, I can't help but wonder if `llvm::Optional<TypeSystem &>` would be more appropriate. There are plenty of instances where it's not a hard error if you can't get a TypeSystem and the appropriate action is probably just to log and move on. I am conflicted because I like how Expected forces you to be more rigorous with error handling but I can't help but feel it is the wrong abstraction. Thoughts?
> >
> >
> > I think an `Optional` would be fine. We can always create an `Error` when necessary, with the trade-off of being a little less precision about the root cause, which honestly doesn't seem that informative anyway.
>
>
> I'm not that familiar with type systems, so I don't know if an explicit error value is useful. Adopting Error/Expected initially has a slightly higher barrier because the surrounding code does not know about Errors, and so you have to do something explicit and verbose to the error, and then create a default value of the return type anyway. But, as the surrounding code adopts Errors, the verboseness should disappear. This, of course, assumes that we want to adopt Errors here and in the surrounding code.
>
> What I am familiar with though is the `Optional` template, I can note that there is no such thing as an `Optional<T&>` (the Optional template does not play well with references). You could make that a thing, but an `Optional<T&>` is essentially just a `T *`, which is what we have now :D. And I wouldn't want `Optional<T*>` as that just creates more ambiguity.
>
> Thinking ahead I am wondering if we could just arrange things such that the TypeSystem creation always succeeds. One day, I am hoping to arrange is so that a Module will always have an associated ObjectFile and a SymbolFile (by avoiding creating a Module if the ObjectFile construction fails, and creating an "empty" SymbolFile if needed). At that point being able to always create a type system would not seem unreasonable. I don't think any of this is directly relevant here and now, but it may speak against introducing Expected here (if you agree with my vision, that is).

Yeah, switching to an Optional<T&> or Optional<T*> doesn't seem like an actual tangible improvement over what we have now... And an Expected<T&> seems to at least promote better error handling.  The initial cost is indeed high though. :P
Eventually having a system such that makes TypeSystem creation always succeed would be much nicer. Though I agree, I think that using an Expected here is better than returning a pointer since TypeSystem creation can fail right now. I don't think it should be too difficult to change back if and when we get to that point.

In D65122#1602853 <https://reviews.llvm.org/D65122#1602853>, @jingham wrote:

> It seems to me part of the goal of Alex's efforts is to make it possible for a given lldb to have or not have support for any particular type system.  In some future day they might even be live pluggable, so that which ones get loaded would be a user choice.  In that future, it is never an error to not have a given type system.  And even if lldb has builtin code to support a given type system, I may not want to pay the cost to construct it.  If I'm debugging ObjC code in a program that also supports swift, I might want to tell lldb not to bother with swift types.
>
> If that seems reasonable, then TypeSystems are really optional, and you should always be prepared for one not to exist.  IIUC that's better expressed by Optional than Expected.

Yes, that is definitely a part of my goal. That's why I felt Optional better suited this than Expected... but I also feel that Expected forces people to be more careful.

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

https://reviews.llvm.org/D65122