Index: cfe/trunk/include/clang/Tooling/Core/Replacement.h =================================================================== --- cfe/trunk/include/clang/Tooling/Core/Replacement.h +++ cfe/trunk/include/clang/Tooling/Core/Replacement.h @@ -22,11 +22,14 @@ #include "clang/Basic/FileManager.h" #include "clang/Basic/LangOptions.h" #include "clang/Basic/SourceLocation.h" +#include "llvm/ADT/Optional.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" +#include "llvm/Support/raw_ostream.h" #include #include #include +#include #include namespace clang { @@ -137,6 +140,59 @@ std::string ReplacementText; }; +enum class replacement_error { + fail_to_apply = 0, + wrong_file_path, + overlap_conflict, + insert_conflict, +}; + +/// \brief Carries extra error information in replacement-related llvm::Error, +/// e.g. fail applying replacements and replacements conflict. +class ReplacementError : public llvm::ErrorInfo { +public: + ReplacementError(replacement_error Err) : Err(Err) {} + + /// \brief Constructs an error related to an existing replacement. + ReplacementError(replacement_error Err, Replacement Existing) + : Err(Err), ExistingReplacement(std::move(Existing)) {} + + /// \brief Constructs an error related to a new replacement and an existing + /// replacement in a set of replacements. + ReplacementError(replacement_error Err, Replacement New, Replacement Existing) + : Err(Err), NewReplacement(std::move(New)), + ExistingReplacement(std::move(Existing)) {} + + std::string message() const override; + + void log(raw_ostream &OS) const override { OS << message(); } + + replacement_error get() const { return Err; } + + static char ID; + + const llvm::Optional &getNewReplacement() const { + return NewReplacement; + } + + const llvm::Optional &getExistingReplacement() const { + return ExistingReplacement; + } + +private: + // Users are not expected to use error_code. + std::error_code convertToErrorCode() const override { + return llvm::inconvertibleErrorCode(); + } + + replacement_error Err; + // A new replacement, which is to expected be added into a set of + // replacements, that is causing problem. + llvm::Optional NewReplacement; + // An existing replacement in a replacements set that is causing problem. + llvm::Optional ExistingReplacement; +}; + /// \brief Less-than operator between two Replacements. bool operator<(const Replacement &LHS, const Replacement &RHS); Index: cfe/trunk/lib/Tooling/Core/Replacement.cpp =================================================================== --- cfe/trunk/lib/Tooling/Core/Replacement.cpp +++ cfe/trunk/lib/Tooling/Core/Replacement.cpp @@ -30,8 +30,7 @@ static const char * const InvalidLocation = ""; -Replacement::Replacement() - : FilePath(InvalidLocation) {} +Replacement::Replacement() : FilePath(InvalidLocation) {} Replacement::Replacement(StringRef FilePath, unsigned Offset, unsigned Length, StringRef ReplacementText) @@ -144,14 +143,33 @@ R.getReplacementText()); } -static llvm::Error makeConflictReplacementsError(const Replacement &New, - const Replacement &Existing) { - return llvm::make_error( - "New replacement:\n" + New.toString() + - "\nconflicts with existing replacement:\n" + Existing.toString(), - llvm::inconvertibleErrorCode()); +static std::string getReplacementErrString(replacement_error Err) { + switch (Err) { + case replacement_error::fail_to_apply: + return "Failed to apply a replacement."; + case replacement_error::wrong_file_path: + return "The new replacement's file path is different from the file path of " + "existing replacements"; + case replacement_error::overlap_conflict: + return "The new replacement overlaps with an existing replacement."; + case replacement_error::insert_conflict: + return "The new insertion has the same insert location as an existing " + "replacement."; + } + llvm_unreachable("A value of replacement_error has no message."); +} + +std::string ReplacementError::message() const { + std::string Message = getReplacementErrString(Err); + if (NewReplacement.hasValue()) + Message += "\nNew replacement: " + NewReplacement->toString(); + if (ExistingReplacement.hasValue()) + Message += "\nExisting replacement: " + ExistingReplacement->toString(); + return Message; } +char ReplacementError::ID = 0; + Replacements Replacements::getCanonicalReplacements() const { std::vector NewReplaces; // Merge adjacent replacements. @@ -202,17 +220,15 @@ if (MergeShiftedRs.getCanonicalReplacements() == MergeShiftedReplaces.getCanonicalReplacements()) return MergeShiftedRs; - return makeConflictReplacementsError(R, *Replaces.begin()); + return llvm::make_error(replacement_error::overlap_conflict, + R, *Replaces.begin()); } llvm::Error Replacements::add(const Replacement &R) { // Check the file path. if (!Replaces.empty() && R.getFilePath() != Replaces.begin()->getFilePath()) - return llvm::make_error( - "All replacements must have the same file path. New replacement: " + - R.getFilePath() + ", existing replacements: " + - Replaces.begin()->getFilePath() + "\n", - llvm::inconvertibleErrorCode()); + return llvm::make_error( + replacement_error::wrong_file_path, R, *Replaces.begin()); // Special-case header insertions. if (R.getOffset() == UINT_MAX) { @@ -239,9 +255,9 @@ // Check if two insertions are order-indepedent: if inserting them in // either order produces the same text, they are order-independent. if ((R.getReplacementText() + I->getReplacementText()).str() != - (I->getReplacementText() + R.getReplacementText()).str()) { - return makeConflictReplacementsError(R, *I); - } + (I->getReplacementText() + R.getReplacementText()).str()) + return llvm::make_error( + replacement_error::insert_conflict, R, *I); // If insertions are order-independent, we can merge them. Replacement NewR( R.getFilePath(), R.getOffset(), 0, @@ -555,9 +571,8 @@ Replacement Replace("", I->getOffset(), I->getLength(), I->getReplacementText()); if (!Replace.apply(Rewrite)) - return llvm::make_error( - "Failed to apply replacement: " + Replace.toString(), - llvm::inconvertibleErrorCode()); + return llvm::make_error( + replacement_error::fail_to_apply, Replace); } std::string Result; llvm::raw_string_ostream OS(Result); Index: cfe/trunk/unittests/Tooling/RefactoringTest.cpp =================================================================== --- cfe/trunk/unittests/Tooling/RefactoringTest.cpp +++ cfe/trunk/unittests/Tooling/RefactoringTest.cpp @@ -100,21 +100,71 @@ EXPECT_TRUE(Replace2.getFilePath().empty()); } +// Checks that an llvm::Error instance contains a ReplacementError with expected +// error code, expected new replacement, and expected existing replacement. +static bool checkReplacementError( + llvm::Error&& Error, replacement_error ExpectedErr, + llvm::Optional ExpectedExisting, + llvm::Optional ExpectedNew) { + if (!Error) { + llvm::errs() << "Error is a success."; + return false; + } + std::string ErrorMessage; + llvm::raw_string_ostream OS(ErrorMessage); + llvm::handleAllErrors(std::move(Error), [&](const ReplacementError &RE) { + llvm::errs() << "Handling error...\n"; + if (ExpectedErr != RE.get()) + OS << "Unexpected error code: " << int(RE.get()) << "\n"; + if (ExpectedExisting != RE.getExistingReplacement()) { + OS << "Expected Existing != Actual Existing.\n"; + if (ExpectedExisting.hasValue()) + OS << "Expected existing replacement: " << ExpectedExisting->toString() + << "\n"; + if (RE.getExistingReplacement().hasValue()) + OS << "Actual existing replacement: " + << RE.getExistingReplacement()->toString() << "\n"; + } + if (ExpectedNew != RE.getNewReplacement()) { + OS << "Expected New != Actual New.\n"; + if (ExpectedNew.hasValue()) + OS << "Expected new replacement: " << ExpectedNew->toString() << "\n"; + if (RE.getNewReplacement().hasValue()) + OS << "Actual new replacement: " << RE.getNewReplacement()->toString() + << "\n"; + } + }); + OS.flush(); + if (ErrorMessage.empty()) return true; + llvm::errs() << ErrorMessage; + return false; +} + TEST_F(ReplacementTest, FailAddReplacements) { Replacements Replaces; Replacement Deletion("x.cc", 0, 10, "3"); auto Err = Replaces.add(Deletion); EXPECT_TRUE(!Err); llvm::consumeError(std::move(Err)); - Err = Replaces.add(Replacement("x.cc", 0, 2, "a")); - EXPECT_TRUE((bool)Err); - llvm::consumeError(std::move(Err)); - Err = Replaces.add(Replacement("x.cc", 2, 2, "a")); - EXPECT_TRUE((bool)Err); - llvm::consumeError(std::move(Err)); - Err = Replaces.add(Replacement("y.cc", 20, 2, "")); - EXPECT_TRUE((bool)Err); - llvm::consumeError(std::move(Err)); + + Replacement OverlappingReplacement("x.cc", 0, 2, "a"); + Err = Replaces.add(OverlappingReplacement); + EXPECT_TRUE(checkReplacementError(std::move(Err), + replacement_error::overlap_conflict, + Deletion, OverlappingReplacement)); + + Replacement ContainedReplacement("x.cc", 2, 2, "a"); + Err = Replaces.add(Replacement(ContainedReplacement)); + EXPECT_TRUE(checkReplacementError(std::move(Err), + replacement_error::overlap_conflict, + Deletion, ContainedReplacement)); + + Replacement WrongPathReplacement("y.cc", 20, 2, ""); + Err = Replaces.add(WrongPathReplacement); + EXPECT_TRUE(checkReplacementError(std::move(Err), + replacement_error::wrong_file_path, + Deletion, WrongPathReplacement)); + EXPECT_EQ(1u, Replaces.size()); EXPECT_EQ(Deletion, *Replaces.begin()); } @@ -299,9 +349,11 @@ // Make sure we find the overlap with the first entry when inserting a // replacement that ends exactly at the seam of the existing replacements. - Err = Replaces.add(Replacement("x.cc", 5, 5, "fail")); - EXPECT_TRUE((bool)Err); - llvm::consumeError(std::move(Err)); + Replacement OverlappingReplacement("x.cc", 5, 5, "fail"); + Err = Replaces.add(OverlappingReplacement); + EXPECT_TRUE(checkReplacementError(std::move(Err), + replacement_error::overlap_conflict, + *Replaces.begin(), OverlappingReplacement)); Err = Replaces.add(Replacement("x.cc", 10, 0, "")); EXPECT_TRUE(!Err); @@ -333,9 +385,11 @@ auto Err = Replaces.add(Replacement("x.cc", 10, 0, "a")); EXPECT_TRUE(!Err); llvm::consumeError(std::move(Err)); - Err = Replaces.add(Replacement("x.cc", 10, 0, "b")); - EXPECT_TRUE((bool)Err); - llvm::consumeError(std::move(Err)); + Replacement ConflictInsertion("x.cc", 10, 0, "b"); + Err = Replaces.add(ConflictInsertion); + EXPECT_TRUE(checkReplacementError(std::move(Err), + replacement_error::insert_conflict, + *Replaces.begin(), ConflictInsertion)); Replaces.clear(); Err = Replaces.add(Replacement("x.cc", 10, 0, "a")); @@ -436,10 +490,12 @@ auto Replaces = toReplacements({Replacement( Context.Sources, Context.getLocation(ID, 2, 1), 5, "other")}); - auto Err = Replaces.add(Replacement( - Context.Sources, Context.getLocation(ID, 2, 1), 5, "rehto")); - EXPECT_TRUE((bool)Err); - llvm::consumeError(std::move(Err)); + Replacement ConflictReplacement(Context.Sources, + Context.getLocation(ID, 2, 1), 5, "rehto"); + auto Err = Replaces.add(ConflictReplacement); + EXPECT_TRUE(checkReplacementError(std::move(Err), + replacement_error::overlap_conflict, + *Replaces.begin(), ConflictReplacement)); EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite)); EXPECT_EQ("line1\nother\nline3\nline4", Context.getRewrittenText(ID));