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