Switch to consumeError(), since this can crash otherwise.

[srhines]

Summary

If assertions are disabled, but LLVM_ABI_BREAKING_CHANGES is enabled,
this will cause an issue with an unchecked Success. Switching to
consumeError() is the correct way to bypass the check.

[srhines]

https://reviews.llvm.org/D36729 similarly fixes a few more instances of this problem (discovered while running tests for this configuration).

[arphaman]

LGTM

[lhames]

The preferred solution to this is actually to wrap the call with cantFail (See http://llvm.org/docs/ProgrammersManual.html#using-cantfail-to-simplify-safe-callsites) -- it will handle both the assertion and consumption of the value for you, and will simplify calls that return an Expected<T>.

[srhines]

In D36728#842644, @lhames wrote:

The preferred solution to this is actually to wrap the call with cantFail (See http://llvm.org/docs/ProgrammersManual.html#using-cantfail-to-simplify-safe-callsites) -- it will handle both the assertion and consumption of the value for you, and will simplify calls that return an Expected<T>.

Is it ok to drop the assertion in that case (and convert it to a comment)? I didn't want to alter too much of this check, since perhaps the original author(s) were more skeptical about this breaking (hence the assertion). Something like:

// Replacements must not conflict since ranges have been merged.
llvm::cantFail(FakeReplaces.add(...));

[lhames]

Is it ok to drop the assertion in that case (and convert it to a comment)? I didn't want to alter too much of this check, since perhaps the original author(s) were more skeptical about this breaking (hence the assertion). Something like:

// Replacements must not conflict since ranges have been merged.
llvm::cantFail(FakeReplaces.add(...));

Yep - cantFail asserts that the result is success, so your proposed change (with the assertion text above the call) is ideal.

[srhines]

https://reviews.llvm.org/D36806 switches this to cantFail(). Thanks for helping me better understand LLVM's error strategy.

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

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.");
-    (void)Err;
+    llvm::consumeError(std::move(Err));
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }
 
 std::vector<Range> Replacements::getAffectedRanges() const {
   std::vector<Range> ChangedRanges;
   int Shift = 0;
   for (const Replacement &R : Replaces) {