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

[srhines]

Ping.

[hintonda]

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]

lib/Tooling/Core/Replacement.cpp

505 ↗

(On Diff #111414)

The text is now in a comment above the call.

[hintonda]

lib/Tooling/Core/Replacement.cpp

505 ↗

(On Diff #111414)

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

[srhines]

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]

lib/Tooling/Core/Replacement.cpp

505 ↗

(On Diff #111414)

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

[hintonda]

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

[srhines]

Any other comments?

[lhames]

In D36806#848246, @hintonda wrote:

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.

[lhames]

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

[srhines]

Switch to using ErrMsg in cantFail().

[srhines]

In D36806#856070, @lhames wrote:

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.

[srhines]

Ping.

[srhines]

Ping

[srhines]

Ping.

[srhines]

Ping again. This is really trivial.

[hintonda]

LGTM...

[lhames]

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

[srhines]

In D36806#1385936, @lhames wrote:

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. :)

cfe/trunk/lib/Tooling/Core/Replacement.cpp

calculateRangesAfterReplacements(const Replacements &Replaces,
   //     (unimportant) replacement text of length "length".
   //   - Merge with \p Replaces.
   //   - The new ranges will be the affected ranges of the merged replacements.
   auto MergedRanges = combineAndSortRanges(Ranges);
   if (Replaces.empty())
     return MergedRanges;
   tooling::Replacements FakeReplaces;
   for (const auto &R : MergedRanges) {
-    auto Err = FakeReplaces.add(Replacement(Replaces.begin()->getFilePath(),
-                                            R.getOffset(), R.getLength(),
-                                            std::string(R.getLength(), ' ')));
-    assert(!Err &&
-           "Replacements must not conflict since ranges have been merged.");
-    llvm::consumeError(std::move(Err));
+    llvm::cantFail(
+        FakeReplaces.add(Replacement(Replaces.begin()->getFilePath(),
+                                     std::string(R.getLength(), ' '))),
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }
 
 } // namespace tooling
 } // namespace clang
 
 std::vector<Range> Replacements::getAffectedRanges() const {