aaron.ballman added inline comments.
================
Comment at: unittests/clang-tidy/ClangTidyTest.h:159-160
+ CleannedReplacements = std::move(FormattedReplacements);
+ if (!CleannedReplacements)
+ llvm_unreachable("!CleannedReplacements");
+ } else {
----------------
jdemeule wrote:
> aaron.ballman wrote:
> > This smells like it should be an assert rather than an unreachable.
> Is returning an empty string OK?
> This should let the assertion on the test do the job.
> Is returning an empty string OK?
It seems like a reasonable behavior, but I've not tried it myself.
> This should let the assertion on the test do the job.
If an assertion fails with assertions disabled, the resulting UB is generally
acceptable. Because these are unit tests, I think we can generally rely on the
assertion being enabled if this code is being run.
================
Comment at: unittests/clang-tidy/ClangTidyTest.h:167
+ if (!Result) {
+ // FIXME: propogate the error.
+ llvm::consumeError(Result.takeError());
----------------
jdemeule wrote:
> aaron.ballman wrote:
> > Are you intending to implement this fixme in this patch?
> I suggest to remove the copy/paste containing the fixme. Is-it OK?
> Maybe with some hints I can suggest a fix for this later.
Ah, I didn't see that this was a copy paste from below. I think those code
paths could be combined if you hoisted the code out to the function level.
https://reviews.llvm.org/D43500
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
