jfb added a comment. In D63518#1558197 <https://reviews.llvm.org/D63518#1558197>, @lhames wrote:
> I haven't had a chance to audit the whole patch yet, but in general the error > suppression idioms are unsafe (though maybe no more so than the existing > code?). > > I would be inclined to audit all those FIXMEs and replace them with cantFails > or consumeErrors. consumeError will generally match existing behavior in > places where you were ignoring errors. cantFail will get you aggressive > crashes, which can be handy for debugging but not so fun in release code. I haven't addressed the comments yet, but wanted to respond: yes, it's unsafe and I was wondering what folks would rather see, so thanks for bringing it up! I indeed only consumed errors that we encountered, and in that sense the code isn't bug-compatible with what we had before. Seems like you'd want `consumeError` in most places, which I can certainly do. > Also, if this patch passes the regression tests we need more failure tests. :) Indeed! That's a pretty terrifying thing... but I'm not signing up to address *that* particular issue :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63518/new/ https://reviews.llvm.org/D63518 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
