NFC change to handle it more consistently in line with other errors, though I'd prefer a quick review since I'm not sure if there's a reason it's was written like that.
(Tidying up parts of MCStreamer code before a bigger differential)
Differential D56245
Use llvm_unreachable instead of errs+abort in MCStreamer.cpp (in EmitRawTextImpl). NFC. kristina on Jan 2 2019, 10:34 PM. Authored by
Details NFC change to handle it more consistently in line with other errors, though I'd prefer a quick review since I'm not sure if there's a reason it's was written like that. (Tidying up parts of MCStreamer code before a bigger differential)
Diff Detail
Event TimelineComment Actions I don't think this should be llvm_unreachable, I think it should be report_fatal_error. LLVM has some some support for out of tree backends, and they could plausibly call this when using a release build of LLVM as a library. If all the callers were in-tree, then calling this would represent an LLVM-internal logic error (forgetting to check hasRawTextSupport()), and unreachable or assert would be appropriate. Comment Actions Revised to be a fatal error after discussion and added comment for any future maintainers. Also made empty braced blocks consistent. Comment Actions Not a strong disagreement, but I'm not sure I follow this entirely - even for external users, there are preconditions for certain API calls that must be met & can still be considered assertion-level failures (ie: the calling code is buggy if it makes this call, not "the calling code can expect a specific failure behavior & rely on this if it wants to") & sounds like this could be one of those. |