zturner added a comment. In D59911#1446942 <https://reviews.llvm.org/D59911#1446942>, @jingham wrote:
> In D59911#1446745 <https://reviews.llvm.org/D59911#1446745>, @davide wrote: > > > My (maybe unpopolar) opinion on the subject is that "soft assertions" are a > > way to cleanse your conscience of guilt, but they don't work really well in > > practice. > > When I started working on lldb, I was a fairly strong proponent of > > assertions everywhere. My view changed somewhat radically over the course > > of the past 18 months, and I would like to summarize some points here. > > > > 1. Vendors (some) ship a release or two of debuggers every 6 months. Even > > if you just look at open source, llvm gets released every 6 months and > > distributions downstream package just the release version. Not everybody > > runs gentoo or exherbo or compiles his toolchain from svn. For these people > > the debugger has just to work. Assertions are always compiled out in > > release mode so this is not an issue. I strongly agree that for internal > > interfaces they have to be used as liberally as possible, mostly because > > they catch bugs in development. llvm libraries tend to use assertions > > galore, and developers put them in "just in case", and I think this is > > really not something we should do in lldb. > > 2. Errors have to be handled, properly. I don't think returning a default > > constructed object in case something goes bad is better than throwing an > > error. We now have rich/proper error handling in llvm to make sure the > > process blows up if the error is not checked. This is a good thing. > > 3. The above points are IMHO the only two needed ways of handling > > invariants/invalid input. Anything else in the middle is just going to > > cause confusion to new and existing developer. Adrian (or anybody else), do > > you have any real example where something wasn't either a user error or a > > strong invariant that couldn't be violated? I personally didn't, hence I > > don't understand the need for "soft" assertions here. > > > Here is a concrete example of the sort of thing I thought lldbassert was for. > In DWARFExpression::AddressRangeForLocationListEntry if we come across a > LocationList format we don't understand, we do: > > default: > // Not supported entry type > lldbassert(false && "Not supported location list type"); > return false; IMO, this example should be changed to return llvm::make_error<DWARFError>("Unsupported location list type"); and let someone higher up handle this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59911/new/ https://reviews.llvm.org/D59911 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
