-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Replace assert
with llvm_unreachable
#32707
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -209,6 +209,7 @@ class LLVM_LIBRARY_VISIBILITY LinearLifetimeChecker::ErrorBuilder { | |||
|
|||
llvm::errs() << "Found ownership error?!\n"; | |||
assert(0 && "triggering standard assertion failure routine"); | |||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm_unreachable("message")
is the canonical way to write assert(0 && "message");
, and should avoid this failure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will llvm_unreachable()
avoid the PrettyStackTrace swallowing?
#32500
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea. It will be fine. I don't remember why I did it this way. I was pretty working quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem in that other one was llvm::report_fatal_error.
Can you change this to llvm_unreachable and then LGTM? |
assert
with llvm_unreachable
@swift-ci smoke test and merge |
1 similar comment
@swift-ci smoke test and merge |
@swift-ci smoke test |
Add an explicit return of true to ensure all paths return a value. Even though this code path is effectively unreachable, it resolves the build error: