Skip to content

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

Merged
merged 3 commits into from
Jul 7, 2020

Conversation

texasmichelle
Copy link
Contributor

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:

In file included from swift/lib/SIL/Verifier/LinearLifetimeChecker.cpp:24:
swift/lib/SIL/Verifier/LinearLifetimeCheckerPrivate.h:212:3: error: non-void function does not return a value in all control paths [-Werror,-Wreturn-type]
  }
  ^

@texasmichelle texasmichelle requested a review from gottesmm July 4, 2020 00:04
@@ -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;
Copy link
Member

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

Copy link
Contributor Author

@texasmichelle texasmichelle Jul 4, 2020

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

Copy link
Contributor

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.

Copy link
Contributor

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.

@gottesmm
Copy link
Contributor

gottesmm commented Jul 4, 2020

Can you change this to llvm_unreachable and then LGTM?

@texasmichelle texasmichelle changed the title Add explicit return true Replace assert with llvm_unreachable Jul 6, 2020
@gottesmm
Copy link
Contributor

gottesmm commented Jul 6, 2020

@swift-ci smoke test and merge

1 similar comment
@gottesmm
Copy link
Contributor

gottesmm commented Jul 7, 2020

@swift-ci smoke test and merge

@CodaFi
Copy link
Contributor

CodaFi commented Jul 7, 2020

@swift-ci smoke test

@texasmichelle texasmichelle merged commit a11c06c into swiftlang:master Jul 7, 2020
@texasmichelle texasmichelle deleted the explicit_true branch July 7, 2020 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants