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