Page MenuHomePhabricator

Switch to cantFail(), since it does the same assertion.
ClosedPublic

Authored by srhines on Aug 16 2017, 1:35 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

srhines created this revision.Aug 16 2017, 1:35 PM
hintonda added inline comments.
lib/Tooling/Core/Replacement.cpp
505 ↗(On Diff #111414)

While obviously correct, are you concerned that by removing the explanatory text, this change will obscure the reason for the assert?

srhines marked an inline comment as done.Aug 21 2017, 1:48 PM
srhines added inline comments.
lib/Tooling/Core/Replacement.cpp
505 ↗(On Diff #111414)

The text is now in a comment above the call.

hintonda added inline comments.Aug 21 2017, 1:57 PM
lib/Tooling/Core/Replacement.cpp
505 ↗(On Diff #111414)

Well, that's what I mean. The reason is no longer in the backtrace.

srhines marked an inline comment as done.Aug 21 2017, 4:27 PM
srhines added inline comments.
lib/Tooling/Core/Replacement.cpp
505 ↗(On Diff #111414)

The backtrace will point to this exact line, so I assume anyone debugging it will eventually read the comment. It might be better to have an optional message to cantFail(), but that isn't within the scope of this change.

hintonda added inline comments.Aug 21 2017, 4:45 PM
lib/Tooling/Core/Replacement.cpp
505 ↗(On Diff #111414)

Sorry, I meant the output of llvm::sys::PrintStackTrace(), which include the assert.

It's just too bad llvm::cantFail() doesn't take an optional string.
But the code is cleaner, so I won't comment further.

Any other comments?

lhames edited edge metadata.Aug 28 2017, 10:34 AM

It's just too bad llvm::cantFail() doesn't take an optional string.
But the code is cleaner, so I won't comment further.

That's not a bad idea actually. Let me add an optional error message to cantFail for you.

I've added an optional ErrMsg argument to cantFail in r312066 - you can now use this to preserve your explanatory text.

srhines updated this revision to Diff 113351.Aug 30 2017, 7:04 PM

Switch to using ErrMsg in cantFail().

I've added an optional ErrMsg argument to cantFail in r312066 - you can now use this to preserve your explanatory text.

Thank you for adding this! I updated the CL to make use of it.

Ping again. This is really trivial.

lhames accepted this revision.Feb 5 2019, 1:17 PM

Looks like this was LGTM'd but never applied. Stephen -- do you have commit access?

This revision is now accepted and ready to land.Feb 5 2019, 1:17 PM

Looks like this was LGTM'd but never applied. Stephen -- do you have commit access?

Yeah, I was waiting on someone with approval for this area of the code to comment and then lost track of it. I can recheck it tonight and then submit it. Thank you for going back through these patches. :)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 9:59 AM