[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL353318: Switch to cantFail(), since it does the same 
assertion. (authored by srhines, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36806/new/

https://reviews.llvm.org/D36806

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


Index: cfe/trunk/lib/Tooling/Core/Replacement.cpp
===
--- cfe/trunk/lib/Tooling/Core/Replacement.cpp
+++ cfe/trunk/lib/Tooling/Core/Replacement.cpp
@@ -519,12 +519,11 @@
 return MergedRanges;
   tooling::Replacements FakeReplaces;
   for (const auto  : 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(),
+ R.getOffset(), R.getLength(),
+ std::string(R.getLength(), ' '))),
+"Replacements must not conflict since ranges have been merged.");
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }


Index: cfe/trunk/lib/Tooling/Core/Replacement.cpp
===
--- cfe/trunk/lib/Tooling/Core/Replacement.cpp
+++ cfe/trunk/lib/Tooling/Core/Replacement.cpp
@@ -519,12 +519,11 @@
 return MergedRanges;
   tooling::Replacements FakeReplaces;
   for (const auto  : 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(),
+ R.getOffset(), R.getLength(),
+ std::string(R.getLength(), ' '))),
+"Replacements must not conflict since ranges have been merged.");
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36806/new/

https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Lang Hames via Phabricator via cfe-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36806/new/

https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

LGTM...


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Ping again. This is really trivial.


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Ping.


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Ping


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Ping.


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines updated this revision to Diff 113351.
srhines added a comment.

Switch to using ErrMsg in cantFail().


https://reviews.llvm.org/D36806

Files:
  lib/Tooling/Core/Replacement.cpp


Index: lib/Tooling/Core/Replacement.cpp
===
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -498,12 +498,11 @@
 return MergedRanges;
   tooling::Replacements FakeReplaces;
   for (const auto  : 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(),
+ R.getOffset(), R.getLength(),
+ std::string(R.getLength(), ' '))),
+"Replacements must not conflict since ranges have been merged.");
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }


Index: lib/Tooling/Core/Replacement.cpp
===
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -498,12 +498,11 @@
 return MergedRanges;
   tooling::Replacements FakeReplaces;
   for (const auto  : 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(),
+ R.getOffset(), R.getLength(),
+ std::string(R.getLength(), ' '))),
+"Replacements must not conflict since ranges have been merged.");
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Lang Hames via Phabricator via cfe-commits]

lhames added a comment.

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


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Lang Hames via Phabricator via cfe-commits]

lhames added a comment.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Any other comments?


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[don hinton via Phabricator via cfe-commits]

hintonda added a comment.

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


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[don hinton via Phabricator via cfe-commits]

hintonda added inline comments.



Comment at: lib/Tooling/Core/Replacement.cpp:505
-assert(!Err &&
-   "Replacements must not conflict since ranges have been merged.");
-llvm::consumeError(std::move(Err));

srhines wrote:
> hintonda wrote:
> > srhines wrote:
> > > hintonda wrote:
> > > > While obviously correct, are you concerned that by removing the 
> > > > explanatory text, this change will obscure the reason for the assert?
> > > The text is now in a comment above the call.
> > Well, that's what I mean.  The reason is no longer in the backtrace.
> 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.
Sorry, I meant the output of llvm::sys::PrintStackTrace(), which include the 
assert.


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines marked an inline comment as done.
srhines added inline comments.



Comment at: lib/Tooling/Core/Replacement.cpp:505
-assert(!Err &&
-   "Replacements must not conflict since ranges have been merged.");
-llvm::consumeError(std::move(Err));

hintonda wrote:
> srhines wrote:
> > hintonda wrote:
> > > While obviously correct, are you concerned that by removing the 
> > > explanatory text, this change will obscure the reason for the assert?
> > The text is now in a comment above the call.
> Well, that's what I mean.  The reason is no longer in the backtrace.
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.


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[don hinton via Phabricator via cfe-commits]

hintonda added inline comments.



Comment at: lib/Tooling/Core/Replacement.cpp:505
-assert(!Err &&
-   "Replacements must not conflict since ranges have been merged.");
-llvm::consumeError(std::move(Err));

srhines wrote:
> hintonda wrote:
> > While obviously correct, are you concerned that by removing the explanatory 
> > text, this change will obscure the reason for the assert?
> The text is now in a comment above the call.
Well, that's what I mean.  The reason is no longer in the backtrace.


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines marked an inline comment as done.
srhines added inline comments.



Comment at: lib/Tooling/Core/Replacement.cpp:505
-assert(!Err &&
-   "Replacements must not conflict since ranges have been merged.");
-llvm::consumeError(std::move(Err));

hintonda wrote:
> While obviously correct, are you concerned that by removing the explanatory 
> text, this change will obscure the reason for the assert?
The text is now in a comment above the call.


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[don hinton via Phabricator via cfe-commits]

hintonda added inline comments.



Comment at: lib/Tooling/Core/Replacement.cpp:505
-assert(!Err &&
-   "Replacements must not conflict since ranges have been merged.");
-llvm::consumeError(std::move(Err));

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


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Ping.


https://reviews.llvm.org/D36806



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines created this revision.
Herald added a subscriber: klimek.

https://reviews.llvm.org/D36806

Files:
  lib/Tooling/Core/Replacement.cpp


Index: lib/Tooling/Core/Replacement.cpp
===
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -498,12 +498,10 @@
 return MergedRanges;
   tooling::Replacements FakeReplaces;
   for (const auto  : 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));
+// Replacements must not conflict since ranges have been merged.
+llvm::cantFail(FakeReplaces.add(
+Replacement(Replaces.begin()->getFilePath(), R.getOffset(),
+R.getLength(), std::string(R.getLength(), ' ';
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }


Index: lib/Tooling/Core/Replacement.cpp
===
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -498,12 +498,10 @@
 return MergedRanges;
   tooling::Replacements FakeReplaces;
   for (const auto  : 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));
+// Replacements must not conflict since ranges have been merged.
+llvm::cantFail(FakeReplaces.add(
+Replacement(Replaces.begin()->getFilePath(), R.getOffset(),
+R.getLength(), std::string(R.getLength(), ' ';
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits