+1. I’ve seen some of the changes in llvm that have changed asserts to return a default value. IMHO these should be changed to return Expected<T> instead On Tue, Mar 12, 2019 at 1:16 PM Jonas Devlieghere via Phabricator < revi...@reviews.llvm.org> wrote:
> JDevlieghere added a comment. > > In D59235#1426169 <https://reviews.llvm.org/D59235#1426169>, @probinson > wrote: > > > In D59235#1425443 <https://reviews.llvm.org/D59235#1425443>, @zturner > wrote: > > > > > In D59235#1425436 <https://reviews.llvm.org/D59235#1425436>, > @clayborg wrote: > > > > > > > My main concern with the LLVM DWARF parser is all of the asserts in > the code. If you attempt to use a DWARFDIE without first checking it for > validity, it will crash on you instead of returning a good error or default > value. That makes me really nervous as we shouldn't just crash the > debugger. The switching over won't be too hard, just the fallout from the > LLDB versions of the class that do error checking and return good > error/default values and LLVM being very strict. > > > > > > > > > Sure, I'm prepared to deal all that appropriately. I don't plan to > regress LLDB's stability in the process. > > > > > > That's why for now I'm just doing very small preliminary steps to get > the two interfaces to be closer to each other and simplify the problem > space. We can worry about the asserts and all of that when we actually > start moving pieces of LLDB to use LLVM's classes (which isn't in this > patch). > > > > > > A long term plan of moving LLVM's parser away from asserts and toward > error reporting on bad input would also make the binutils that try to read > DWARF more robust and useful for trying to diagnose bad object files. I'm > all for it. > > > Agreed, and we've been doing this for new patches for a while now. > However, I very strongly prefer having asserts over "returning a default > value", which only hides real bugs. > > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D59235/new/ > > https://reviews.llvm.org/D59235 > > > >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
