dblaikie added a comment.
I thought this was settled quite a while ago and enshrined in the style guide:
https://llvm.org/docs/CodingStandards.html#assert-liberally
`assert(0)` should not be used if something is reachable. We shouldn't have a
"this violates an invariant, but if you don't have asserts enabled you do get
some maybe-acceptable behavior".
I feel fairly strongly that any cases of "reachable asserts" should be changed
to valid error handling or `llvm_report_error` and remaining `assert(0)` should
be transformed to `llvm_unreachable`. (also, ideally, don't use
branch-to-`unreachable` where possible, instead assert the condition - in cases
where the `if` has side effects, sometimes that's the nicest way to write it,
but might be clearer, if more verbose to use a local variable for the
condition, then assert that the variable is true (and have the requisite
"variable might be unused" cast))
> Historically, we've usually used llvm_unreachable for situations where we're
> saying "this code cannot be reached; if it can, something else has gone
> seriously wrong." For example, in code like: int foo(SomeEnum E) { switch (E)
> { case One: return 1; default: return 2; } llvm_unreachable("getting here
> would be mysterious"); } and we've used assert(0) for situations where we're
> saying "this code is possible to reach only when there were mistakes
> elsewhere which broke invariants we were relying on."
I don't think those are different things though - violating invariants is ~=
something going seriously wrong.
(sorry, I guess I should debate this on the thread instead of here - but I
think most of the folks on that thread did agree with the LLVM style guide/the
direction here)
This, I think was also discussed about a decade ago in the LLVM community and
resulted in r166821/2962d9599e463265edae599285bbc6351f1cc0ef which specifically
"Suggests llvm_unreachable over assert(0)" and is the policy of LLVM - this
change is consistent with that policy.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135551/new/
https://reviews.llvm.org/D135551
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
