[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
This revision was automatically updated to reflect the committed changes.
Closed by commit rL371392: [clangd] Support multifile edits as output of Tweaks
(authored by kadircet, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D66637?vs=219324=219325#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
Files:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/SourceCode.cpp
clang-tools-extra/trunk/clangd/SourceCode.h
clang-tools-extra/trunk/clangd/refactor/Tweak.cpp
clang-tools-extra/trunk/clangd/refactor/Tweak.h
clang-tools-extra/trunk/clangd/refactor/tweaks/AnnotateHighlightings.cpp
clang-tools-extra/trunk/clangd/refactor/tweaks/ExpandAutoType.cpp
clang-tools-extra/trunk/clangd/refactor/tweaks/ExpandMacro.cpp
clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/trunk/clangd/refactor/tweaks/RawStringLiteral.cpp
clang-tools-extra/trunk/clangd/refactor/tweaks/SwapIfBranches.cpp
clang-tools-extra/trunk/clangd/unittests/TweakTesting.cpp
clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/trunk/clangd/SourceCode.cpp
===
--- clang-tools-extra/trunk/clangd/SourceCode.cpp
+++ clang-tools-extra/trunk/clangd/SourceCode.cpp
@@ -11,6 +11,7 @@
#include "FuzzyMatch.h"
#include "Logger.h"
#include "Protocol.h"
+#include "refactor/Tweak.h"
#include "clang/AST/ASTContext.h"
#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h"
@@ -19,14 +20,21 @@
#include "clang/Format/Format.h"
#include "clang/Lex/Lexer.h"
#include "clang/Lex/Preprocessor.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/None.h"
#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/Errc.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/LineIterator.h"
+#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
+#include "llvm/Support/SHA1.h"
+#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/Support/xxhash.h"
#include
@@ -562,7 +570,7 @@
}
// Handle the symbolic link path case where the current working directory
- // (getCurrentWorkingDirectory) is a symlink./ We always want to the real
+ // (getCurrentWorkingDirectory) is a symlink. We always want to the real
// file path (instead of the symlink path) for the C++ symbols.
//
// Consider the following example:
@@ -908,5 +916,57 @@
return None;
}
+llvm::Expected Edit::apply() const {
+ return tooling::applyAllReplacements(InitialCode, Replacements);
+}
+
+std::vector Edit::asTextEdits() const {
+ return replacementsToEdits(InitialCode, Replacements);
+}
+
+bool Edit::canApplyTo(llvm::StringRef Code) const {
+ // Create line iterators, since line numbers are important while applying our
+ // edit we cannot skip blank lines.
+ auto LHS = llvm::MemoryBuffer::getMemBuffer(Code);
+ llvm::line_iterator LHSIt(*LHS, /*SkipBlanks=*/false);
+
+ auto RHS = llvm::MemoryBuffer::getMemBuffer(InitialCode);
+ llvm::line_iterator RHSIt(*RHS, /*SkipBlanks=*/false);
+
+ // Compare the InitialCode we prepared the edit for with the Code we received
+ // line by line to make sure there are no differences.
+ // FIXME: This check is too conservative now, it should be enough to only
+ // check lines around the replacements contained inside the Edit.
+ while (!LHSIt.is_at_eof() && !RHSIt.is_at_eof()) {
+if (*LHSIt != *RHSIt)
+ return false;
+++LHSIt;
+++RHSIt;
+ }
+
+ // After we reach EOF for any of the files we make sure the other one doesn't
+ // contain any additional content except empty lines, they should not
+ // interfere with the edit we produced.
+ while (!LHSIt.is_at_eof()) {
+if (!LHSIt->empty())
+ return false;
+++LHSIt;
+ }
+ while (!RHSIt.is_at_eof()) {
+if (!RHSIt->empty())
+ return false;
+++RHSIt;
+ }
+ return true;
+}
+
+llvm::Error reformatEdit(Edit , const format::FormatStyle ) {
+ if (auto NewEdits = cleanupAndFormat(E.InitialCode, E.Replacements, Style))
+E.Replacements = std::move(*NewEdits);
+ else
+return NewEdits.takeError();
+ return llvm::Error::success();
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -12,6 +12,7 @@
#include "Format.h"
#include
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet updated this revision to Diff 219324.
kadircet marked 4 inline comments as done.
kadircet added a comment.
- Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -8,15 +8,26 @@
#include "Annotations.h"
#include "SourceCode.h"
+#include "TestFS.h"
#include "TestTU.h"
#include "TweakTesting.h"
#include "refactor/Tweak.h"
#include "clang/AST/Expr.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock-matchers.h"
#include "gmock/gmock.h"
@@ -31,6 +42,27 @@
namespace clangd {
namespace {
+TEST(FileEdits, AbsolutePath) {
+ auto RelPaths = {"a.h", "foo.cpp", "test/test.cpp"};
+
+ llvm::IntrusiveRefCntPtr MemFS(
+ new llvm::vfs::InMemoryFileSystem);
+ MemFS->setCurrentWorkingDirectory(testRoot());
+ for (auto Path : RelPaths)
+MemFS->addFile(Path, 0, llvm::MemoryBuffer::getMemBuffer("", Path));
+ FileManager FM(FileSystemOptions(), MemFS);
+ DiagnosticsEngine DE(new DiagnosticIDs, new DiagnosticOptions);
+ SourceManager SM(DE, FM);
+
+ for (auto Path : RelPaths) {
+auto FID = SM.createFileID(*FM.getFile(Path), SourceLocation(),
+ clang::SrcMgr::C_User);
+auto Res = Tweak::Effect::fileEdit(SM, FID, tooling::Replacements());
+ASSERT_THAT_EXPECTED(Res, llvm::Succeeded());
+EXPECT_EQ(Res->first, testPath(Path));
+ }
+}
+
TWEAK_TEST(SwapIfBranches);
TEST_F(SwapIfBranchesTest, Test) {
Context = Function;
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -9,8 +9,8 @@
#include "TweakTesting.h"
#include "Annotations.h"
-#include "refactor/Tweak.h"
#include "SourceCode.h"
+#include "refactor/Tweak.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/Support/Error.h"
@@ -98,14 +98,16 @@
return "fail: " + llvm::toString(Result.takeError());
if (Result->ShowMessage)
return "message:\n" + *Result->ShowMessage;
- if (Result->ApplyEdit) {
-if (auto NewText =
-tooling::applyAllReplacements(Input.code(), *Result->ApplyEdit))
- return unwrap(Context, *NewText);
-else
- return "bad edits: " + llvm::toString(NewText.takeError());
- }
- return "no effect";
+ if (Result->ApplyEdits.empty())
+return "no effect";
+ if (Result->ApplyEdits.size() > 1)
+return "received multi-file edits";
+
+ auto ApplyEdit = Result->ApplyEdits.begin()->second;
+ if (auto NewText = ApplyEdit.apply())
+return unwrap(Context, *NewText);
+ else
+return "bad edits: " + llvm::toString(NewText.takeError());
}
::testing::Matcher TweakTest::isAvailable() const {
Index: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -90,7 +90,7 @@
ElseRng->getBegin(),
ElseCode.size(), ThenCode)))
return std::move(Err);
- return Effect::applyEdit(Result);
+ return
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:414
+ if (llvm::Error Err = reformatEdit(E, Style)) {
+llvm::handleAllErrors(std::move(Err), [&](llvm::ErrorInfoBase ) {
+ elog("Failed to format {0}: {1}", It.first(), EIB.message());
sammccall wrote:
> kadircet wrote:
> > sammccall wrote:
> > > isn't this just
> > > `elog("Failed to format {0}: {1}", std::move(Err))`
> > IIUC, just printing the error doesn't actually mark it as checked,
> > therefore causes an assertion failure (see `getPtr` vs `getPayload` in
> > `llvm::Error`). And I couldn't see any special handling in `elog`, but not
> > sure if `formatv object` has this.
> The special handling is on Logger.h:38. Do you see crashes?
ah OK, didn't notice the `fmt_consume` previously. no, I wasn't seeing any
crashes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:414
+ if (llvm::Error Err = reformatEdit(E, Style)) {
+llvm::handleAllErrors(std::move(Err), [&](llvm::ErrorInfoBase ) {
+ elog("Failed to format {0}: {1}", It.first(), EIB.message());
kadircet wrote:
> sammccall wrote:
> > isn't this just
> > `elog("Failed to format {0}: {1}", std::move(Err))`
> IIUC, just printing the error doesn't actually mark it as checked, therefore
> causes an assertion failure (see `getPtr` vs `getPayload` in `llvm::Error`).
> And I couldn't see any special handling in `elog`, but not sure if `formatv
> object` has this.
The special handling is on Logger.h:38. Do you see crashes?
Comment at: clang-tools-extra/clangd/refactor/Tweak.cpp:97
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Failed to get absolute path for: " +
+ SM.getFileEntryForID(FID)->getName());
nit: "for edited file: "...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet updated this revision to Diff 218883.
kadircet added a comment.
- Rebase
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -8,15 +8,26 @@
#include "Annotations.h"
#include "SourceCode.h"
+#include "TestFS.h"
#include "TestTU.h"
#include "TweakTesting.h"
#include "refactor/Tweak.h"
#include "clang/AST/Expr.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock-matchers.h"
#include "gmock/gmock.h"
@@ -31,6 +42,27 @@
namespace clangd {
namespace {
+TEST(FileEdits, AbsolutePath) {
+ auto RelPaths = {"a.h", "foo.cpp", "test/test.cpp"};
+
+ llvm::IntrusiveRefCntPtr MemFS(
+ new llvm::vfs::InMemoryFileSystem);
+ MemFS->setCurrentWorkingDirectory(testRoot());
+ for (auto Path : RelPaths)
+MemFS->addFile(Path, 0, llvm::MemoryBuffer::getMemBuffer("", Path));
+ FileManager FM(FileSystemOptions(), MemFS);
+ DiagnosticsEngine DE(new DiagnosticIDs, new DiagnosticOptions);
+ SourceManager SM(DE, FM);
+
+ for (auto Path : RelPaths) {
+auto FID = SM.createFileID(*FM.getFile(Path), SourceLocation(),
+ clang::SrcMgr::C_User);
+auto Res = Tweak::Effect::fileEdit(SM, FID, tooling::Replacements());
+ASSERT_THAT_EXPECTED(Res, llvm::Succeeded());
+EXPECT_EQ(Res->first, testPath(Path));
+ }
+}
+
TWEAK_TEST(SwapIfBranches);
TEST_F(SwapIfBranchesTest, Test) {
Context = Function;
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -9,8 +9,8 @@
#include "TweakTesting.h"
#include "Annotations.h"
-#include "refactor/Tweak.h"
#include "SourceCode.h"
+#include "refactor/Tweak.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/Support/Error.h"
@@ -98,14 +98,16 @@
return "fail: " + llvm::toString(Result.takeError());
if (Result->ShowMessage)
return "message:\n" + *Result->ShowMessage;
- if (Result->ApplyEdit) {
-if (auto NewText =
-tooling::applyAllReplacements(Input.code(), *Result->ApplyEdit))
- return unwrap(Context, *NewText);
-else
- return "bad edits: " + llvm::toString(NewText.takeError());
- }
- return "no effect";
+ if (Result->ApplyEdits.empty())
+return "no effect";
+ if (Result->ApplyEdits.size() > 1)
+return "received multi-file edits";
+
+ auto ApplyEdit = Result->ApplyEdits.begin()->second;
+ if (auto NewText = ApplyEdit.apply())
+return unwrap(Context, *NewText);
+ else
+return "bad edits: " + llvm::toString(NewText.takeError());
}
::testing::Matcher TweakTest::isAvailable() const {
Index: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -90,7 +90,7 @@
ElseRng->getBegin(),
ElseCode.size(), ThenCode)))
return std::move(Err);
- return Effect::applyEdit(Result);
+ return Effect::mainFileEdit(SrcMgr, std::move(Result));
}
} //
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet added inline comments. Comment at: clang-tools-extra/clangd/refactor/Tweak.h:130 +/// pointed by FID. +Tweak::Effect fileEdit(const SourceManager , FileID FID, + tooling::Replacements Replacements); kadircet wrote: > kadircet wrote: > > sammccall wrote: > > > sammccall wrote: > > > > what will this be used for? > > > > > > > > You can use at most one of these Effect factories (unless we're going > > > > to do some convoluted merge thing), so this seems useful if you've got > > > > exactly one edit to a non-main file. > > > > > > > > It seems more useful to return a pair which could be > > > > inserted into a map > > > why are these moved out of the Effect class? > > > need to provide a fully-qualified name (I don't think the implementations > > > in this patch actually do that) > > > > I've thought you were also requesting these to be helper functions rather > > than static methods so that callers don't need to qualify the call. > > Can move it back into class if I misunderstood your point, but this > > actually seems a lot cleaner. > > what will this be used for? > > you are right it doesn't really compose well, changed it to return the pair > instead. moved back into class Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66637/new/ https://reviews.llvm.org/D66637 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet updated this revision to Diff 218878.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Define more strict semantics around filename
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -7,16 +7,28 @@
//===--===//
#include "Annotations.h"
+#include "ClangdUnit.h"
#include "SourceCode.h"
+#include "TestFS.h"
#include "TestTU.h"
#include "TweakTesting.h"
#include "refactor/Tweak.h"
#include "clang/AST/Expr.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock-matchers.h"
#include "gmock/gmock.h"
@@ -113,11 +125,15 @@
auto Effect = apply(ID, Input);
if (!Effect)
return Effect.takeError();
- if (!Effect->ApplyEdit)
+ if (Effect->ApplyEdits.empty())
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No replacements");
+ auto Edits = Effect->ApplyEdits;
+ if (Edits.size() > 1)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Multi-file edits");
Annotations Code(Input);
- return applyAllReplacements(Code.code(), *Effect->ApplyEdit);
+ return applyAllReplacements(Code.code(), Edits.begin()->second.Replacements);
}
void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
@@ -127,6 +143,27 @@
EXPECT_EQ(Output, std::string(*Result)) << Input;
}
+TEST(FileEdits, AbsolutePath) {
+ auto RelPaths = {"a.h", "foo.cpp", "test/test.cpp"};
+
+ llvm::IntrusiveRefCntPtr MemFS(
+ new llvm::vfs::InMemoryFileSystem);
+ MemFS->setCurrentWorkingDirectory(testRoot());
+ for (auto Path : RelPaths)
+MemFS->addFile(Path, 0, llvm::MemoryBuffer::getMemBuffer("", Path));
+ FileManager FM(FileSystemOptions(), MemFS);
+ DiagnosticsEngine DE(new DiagnosticIDs, new DiagnosticOptions);
+ SourceManager SM(DE, FM);
+
+ for (auto Path : RelPaths) {
+auto FID = SM.createFileID(*FM.getFile(Path), SourceLocation(),
+ clang::SrcMgr::C_User);
+auto Res = Tweak::Effect::fileEdit(SM, FID, tooling::Replacements());
+ASSERT_THAT_EXPECTED(Res, Succeeded());
+EXPECT_EQ(Res->first, testPath(Path));
+ }
+}
+
TWEAK_TEST(SwapIfBranches);
TEST_F(SwapIfBranchesTest, Test) {
Context = Function;
@@ -495,7 +532,7 @@
checkTransform(ID, Input, Output);
checkTransform(ID,
- R"cpp(
+ R"cpp(
[[void f1();
void f2();]]
)cpp",
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -9,8 +9,8 @@
#include "TweakTesting.h"
#include "Annotations.h"
-#include "refactor/Tweak.h"
#include "SourceCode.h"
+#include "refactor/Tweak.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/Support/Error.h"
@@ -98,14 +98,16 @@
return "fail: " + llvm::toString(Result.takeError());
if (Result->ShowMessage)
return "message:\n" + *Result->ShowMessage;
- if (Result->ApplyEdit) {
-if (auto NewText =
-tooling::applyAllReplacements(Input.code(), *Result->ApplyEdit))
- return unwrap(Context, *NewText);
-else
- return "bad edits: " +
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet updated this revision to Diff 218637.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -7,7 +7,9 @@
//===--===//
#include "Annotations.h"
+#include "ClangdUnit.h"
#include "SourceCode.h"
+#include "TestFS.h"
#include "TestTU.h"
#include "TweakTesting.h"
#include "refactor/Tweak.h"
@@ -113,11 +115,15 @@
auto Effect = apply(ID, Input);
if (!Effect)
return Effect.takeError();
- if (!Effect->ApplyEdit)
+ if (Effect->ApplyEdits.empty())
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No replacements");
+ auto Edits = Effect->ApplyEdits;
+ if (Edits.size() > 1)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Multi-file edits");
Annotations Code(Input);
- return applyAllReplacements(Code.code(), *Effect->ApplyEdit);
+ return applyAllReplacements(Code.code(), Edits.begin()->second.Replacements);
}
void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
@@ -127,6 +133,39 @@
EXPECT_EQ(Output, std::string(*Result)) << Input;
}
+TEST(FileEdits, AbsolutePath) {
+ TestTU TU;
+ TU.Filename = "foo.cpp";
+ TU.Code = R"cpp(
+#include "a.h"
+#include "b.h"
+void bar();
+ )cpp";
+ TU.AdditionalFiles = {
+ {"a.h", "void baz();"},
+ {"test/b.h", "void foo();"},
+ };
+ auto T = "-I" + testPath("test/");
+ TU.ExtraArgs = {T.c_str()};
+
+ const ParsedAST AST = TU.build();
+ ASSERT_TRUE(AST.getDiagnostics().empty());
+
+ auto = AST.getSourceManager();
+ const auto Files = {testPath("foo.cpp"), testPath("test/b.h"),
+ testPath("a.h")};
+
+ for (const Decl *D : AST.getASTContext().getTranslationUnitDecl()->decls()) {
+if (D->getLocation().isInvalid() ||
+SM.isWrittenInBuiltinFile(D->getLocation()))
+ continue;
+
+auto FID = SM.getFileID(D->getLocation());
+auto Res = fileEdit(SM, FID, tooling::Replacements());
+EXPECT_THAT(Files, testing::Contains(Res.first));
+ }
+}
+
TWEAK_TEST(SwapIfBranches);
TEST_F(SwapIfBranchesTest, Test) {
Context = Function;
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -9,8 +9,8 @@
#include "TweakTesting.h"
#include "Annotations.h"
-#include "refactor/Tweak.h"
#include "SourceCode.h"
+#include "refactor/Tweak.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/Support/Error.h"
@@ -98,14 +98,16 @@
return "fail: " + llvm::toString(Result.takeError());
if (Result->ShowMessage)
return "message:\n" + *Result->ShowMessage;
- if (Result->ApplyEdit) {
-if (auto NewText =
-tooling::applyAllReplacements(Input.code(), *Result->ApplyEdit))
- return unwrap(Context, *NewText);
-else
- return "bad edits: " + llvm::toString(NewText.takeError());
- }
- return "no effect";
+ if (Result->ApplyEdits.empty())
+return "no effect";
+ if (Result->ApplyEdits.size() > 1)
+return "received multi-file edits";
+
+ auto ApplyEdit = Result->ApplyEdits.begin()->second;
+ if (auto NewText = ApplyEdit.apply())
+return unwrap(Context, *NewText);
+ else
+return "bad edits: " + llvm::toString(NewText.takeError());
}
::testing::Matcher TweakTest::isAvailable() const {
Index: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -90,7 +90,7 @@
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet marked 13 inline comments as done.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:414
+ if (llvm::Error Err = reformatEdit(E, Style)) {
+llvm::handleAllErrors(std::move(Err), [&](llvm::ErrorInfoBase ) {
+ elog("Failed to format {0}: {1}", It.first(), EIB.message());
sammccall wrote:
> isn't this just
> `elog("Failed to format {0}: {1}", std::move(Err))`
IIUC, just printing the error doesn't actually mark it as checked, therefore
causes an assertion failure (see `getPtr` vs `getPayload` in `llvm::Error`).
And I couldn't see any special handling in `elog`, but not sure if `formatv
object` has this.
Comment at: clang-tools-extra/clangd/refactor/Tweak.cpp:90
+ assert(FE && "Generating edits for unknown file!");
+ llvm::StringRef FilePath = FE->getName();
+ Edit Ed(SM.getBufferData(FID), std::move(Replacements));
sammccall wrote:
> how do you know this is the correct/absolute path for the file? Can we add
> tests?
changed it to use `FileInfo::getName` rather then `FileEntry::getName`, as the
former says:
```
/// Returns the name of the file that was used when the file was loaded from
/// the underlying file system.```
These also get tested through any lit tests we have for tweaks, adding more
unittests.
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:130
+/// pointed by FID.
+Tweak::Effect fileEdit(const SourceManager , FileID FID,
+ tooling::Replacements Replacements);
sammccall wrote:
> sammccall wrote:
> > what will this be used for?
> >
> > You can use at most one of these Effect factories (unless we're going to do
> > some convoluted merge thing), so this seems useful if you've got exactly
> > one edit to a non-main file.
> >
> > It seems more useful to return a pair which could be inserted
> > into a map
> why are these moved out of the Effect class?
> need to provide a fully-qualified name (I don't think the implementations in
> this patch actually do that)
I've thought you were also requesting these to be helper functions rather than
static methods so that callers don't need to qualify the call.
Can move it back into class if I misunderstood your point, but this actually
seems a lot cleaner.
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:130
+/// pointed by FID.
+Tweak::Effect fileEdit(const SourceManager , FileID FID,
+ tooling::Replacements Replacements);
kadircet wrote:
> sammccall wrote:
> > sammccall wrote:
> > > what will this be used for?
> > >
> > > You can use at most one of these Effect factories (unless we're going to
> > > do some convoluted merge thing), so this seems useful if you've got
> > > exactly one edit to a non-main file.
> > >
> > > It seems more useful to return a pair which could be
> > > inserted into a map
> > why are these moved out of the Effect class?
> > need to provide a fully-qualified name (I don't think the implementations
> > in this patch actually do that)
>
> I've thought you were also requesting these to be helper functions rather
> than static methods so that callers don't need to qualify the call.
> Can move it back into class if I misunderstood your point, but this actually
> seems a lot cleaner.
> what will this be used for?
you are right it doesn't really compose well, changed it to return the pair
instead.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:647
+ WE.changes.emplace();
+ auto FS = FSProvider.getFileSystem();
+ for (const auto : R->ApplyEdits) {
please pull out a method for this part
e.g. `Error validateEdits(...)`
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:654
+ if (!It.second.canApplyTo(*Draft))
+return Reply((It.first() + " needs to be saved").str());
+}
As these are absolute paths, we probably want to invert the error message for
readability, and at least count the files.
e.g.
"Files must be saved first: path/to/Foo.cpp (and 3 others)"
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:414
+ if (llvm::Error Err = reformatEdit(E, Style)) {
+llvm::handleAllErrors(std::move(Err), [&](llvm::ErrorInfoBase ) {
+ elog("Failed to format {0}: {1}", It.first(), EIB.message());
isn't this just
`elog("Failed to format {0}: {1}", std::move(Err))`
Comment at: clang-tools-extra/clangd/SourceCode.cpp:856
+
+bool Edit::canApplyTo(llvm::StringRef Code) const {
+ auto LHS = llvm::MemoryBuffer::getMemBuffer(Code);
documentation
Comment at: clang-tools-extra/clangd/SourceCode.cpp:870
+
+ // We only allow trailing empty lines.
+ while (!LHSIt.is_at_eof()) {
why?
Comment at: clang-tools-extra/clangd/SourceCode.h:42
+/// A set of edits generated for a single file. Can verify whether it is safe
to
+/// apply these edits to a code block.
move this class near the related code? (`replacementToEdit` etc)
Comment at: clang-tools-extra/clangd/refactor/Tweak.cpp:90
+ assert(FE && "Generating edits for unknown file!");
+ llvm::StringRef FilePath = FE->getName();
+ Edit Ed(SM.getBufferData(FID), std::move(Replacements));
how do you know this is the correct/absolute path for the file? Can we add
tests?
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:73
llvm::Optional ShowMessage;
-/// An edit to apply to the input file.
-llvm::Optional ApplyEdit;
+/// A mapping from absolute file path to edits.
+llvm::StringMap ApplyEdits;
is it possible to be more specific than "absolute"?
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:130
+/// pointed by FID.
+Tweak::Effect fileEdit(const SourceManager , FileID FID,
+ tooling::Replacements Replacements);
what will this be used for?
You can use at most one of these Effect factories (unless we're going to do
some convoluted merge thing), so this seems useful if you've got exactly one
edit to a non-main file.
It seems more useful to return a pair which could be inserted
into a map
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:130
+/// pointed by FID.
+Tweak::Effect fileEdit(const SourceManager , FileID FID,
+ tooling::Replacements Replacements);
sammccall wrote:
> what will this be used for?
>
> You can use at most one of these Effect factories (unless we're going to do
> some convoluted merge thing), so this seems useful if you've got exactly one
> edit to a non-main file.
>
> It seems more useful to return a pair which could be inserted
> into a map
why are these moved out of the Effect class?
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:133
+
+/// Convinience wrapper around above.
+Tweak::Effect mainFileEdit(const SourceManager ,
nit: convenience
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet updated this revision to Diff 217820.
kadircet marked an inline comment as done.
kadircet added a comment.
- Get rid off compatibility check for files that are not open in the editor.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -113,11 +113,15 @@
auto Effect = apply(ID, Input);
if (!Effect)
return Effect.takeError();
- if (!Effect->ApplyEdit)
+ if (Effect->ApplyEdits.empty())
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No replacements");
+ auto Edits = Effect->ApplyEdits;
+ if (Edits.size() > 1)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Multi-file edits");
Annotations Code(Input);
- return applyAllReplacements(Code.code(), *Effect->ApplyEdit);
+ return applyAllReplacements(Code.code(), Edits.begin()->second.Replacements);
}
void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -9,8 +9,8 @@
#include "TweakTesting.h"
#include "Annotations.h"
-#include "refactor/Tweak.h"
#include "SourceCode.h"
+#include "refactor/Tweak.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/Support/Error.h"
@@ -98,14 +98,16 @@
return "fail: " + llvm::toString(Result.takeError());
if (Result->ShowMessage)
return "message:\n" + *Result->ShowMessage;
- if (Result->ApplyEdit) {
-if (auto NewText =
-tooling::applyAllReplacements(Input.code(), *Result->ApplyEdit))
- return unwrap(Context, *NewText);
-else
- return "bad edits: " + llvm::toString(NewText.takeError());
- }
- return "no effect";
+ if (Result->ApplyEdits.empty())
+return "no effect";
+ if (Result->ApplyEdits.size() > 1)
+return "received multi-file edits";
+
+ auto ApplyEdit = Result->ApplyEdits.begin()->second;
+ if (auto NewText = ApplyEdit.apply())
+return unwrap(Context, *NewText);
+ else
+return "bad edits: " + llvm::toString(NewText.takeError());
}
::testing::Matcher TweakTest::isAvailable() const {
Index: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -90,7 +90,7 @@
ElseRng->getBegin(),
ElseCode.size(), ThenCode)))
return std::move(Err);
- return Effect::applyEdit(Result);
+ return mainFileEdit(SrcMgr, std::move(Result));
}
} // namespace
Index: clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
@@ -88,10 +88,12 @@
}
Expected RawStringLiteral::apply(const Selection ) {
- return Effect::applyEdit(tooling::Replacements(
+ auto = Inputs.AST.getSourceManager();
+ auto Reps = tooling::Replacements(
tooling::Replacement(Inputs.AST.getSourceManager(), Str,
("R\"(" + Str->getBytes() + ")\"").str(),
- Inputs.AST.getASTContext().getLangOpts(;
+ Inputs.AST.getASTContext().getLangOpts()));
+ return mainFileEdit(SM, std::move(Reps));
}
} // namespace
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
sammccall added inline comments. Comment at: clang-tools-extra/clangd/SourceCode.cpp:601 + llvm::inconvertibleErrorCode(), + "File contents differ on disk for %s, please save", FilePath.data()); +} kadircet wrote: > sammccall wrote: > > you seem to be checking this both here and in clangdlspserver. Why? > the contents in here are coming from disk, whereas the one in clangdlspserver > is coming from drafts. I suppose it would make more sense to merge the two in > clangdlspserver, use draft manager as content source backed by disk in case > user hasn't opened that file. OK, so you're checking here whether the contents-on-disk have changed between clang reading the file as part of parse, and the tweak completing? I don't think this is worth doing. Sure, there's potentially a race condition, but it's relatively small (bounded on the order of a few seconds, vs unbounded for unsaved drafts) and we can't eliminate it entirely (file on disk can change between when we create edits and when they're applied). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66637/new/ https://reviews.llvm.org/D66637 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet updated this revision to Diff 217806.
kadircet added a comment.
- Handle formatting error in ClangdServer rather than just printing it
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -113,11 +113,15 @@
auto Effect = apply(ID, Input);
if (!Effect)
return Effect.takeError();
- if (!Effect->ApplyEdit)
+ if (Effect->ApplyEdits.empty())
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No replacements");
+ auto Edits = Effect->ApplyEdits;
+ if (Edits.size() > 1)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Multi-file edits");
Annotations Code(Input);
- return applyAllReplacements(Code.code(), *Effect->ApplyEdit);
+ return applyAllReplacements(Code.code(), Edits.begin()->second.Replacements);
}
void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -9,8 +9,8 @@
#include "TweakTesting.h"
#include "Annotations.h"
-#include "refactor/Tweak.h"
#include "SourceCode.h"
+#include "refactor/Tweak.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/Support/Error.h"
@@ -98,14 +98,16 @@
return "fail: " + llvm::toString(Result.takeError());
if (Result->ShowMessage)
return "message:\n" + *Result->ShowMessage;
- if (Result->ApplyEdit) {
-if (auto NewText =
-tooling::applyAllReplacements(Input.code(), *Result->ApplyEdit))
- return unwrap(Context, *NewText);
-else
- return "bad edits: " + llvm::toString(NewText.takeError());
- }
- return "no effect";
+ if (Result->ApplyEdits.empty())
+return "no effect";
+ if (Result->ApplyEdits.size() > 1)
+return "received multi-file edits";
+
+ auto ApplyEdit = Result->ApplyEdits.begin()->second;
+ if (auto NewText = ApplyEdit.apply())
+return unwrap(Context, *NewText);
+ else
+return "bad edits: " + llvm::toString(NewText.takeError());
}
::testing::Matcher TweakTest::isAvailable() const {
Index: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -90,7 +90,7 @@
ElseRng->getBegin(),
ElseCode.size(), ThenCode)))
return std::move(Err);
- return Effect::applyEdit(Result);
+ return mainFileEdit(SrcMgr, std::move(Result));
}
} // namespace
Index: clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
@@ -88,10 +88,12 @@
}
Expected RawStringLiteral::apply(const Selection ) {
- return Effect::applyEdit(tooling::Replacements(
+ auto = Inputs.AST.getSourceManager();
+ auto Reps = tooling::Replacements(
tooling::Replacement(Inputs.AST.getSourceManager(), Str,
("R\"(" + Str->getBytes() + ")\"").str(),
- Inputs.AST.getASTContext().getLangOpts(;
+ Inputs.AST.getASTContext().getLangOpts()));
+ return mainFileEdit(SM, std::move(Reps));
}
} // namespace
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -468,7 +468,7 @@
// replace
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet updated this revision to Diff 217803.
kadircet marked 4 inline comments as done.
kadircet added a comment.
- Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -113,11 +113,15 @@
auto Effect = apply(ID, Input);
if (!Effect)
return Effect.takeError();
- if (!Effect->ApplyEdit)
+ if (Effect->ApplyEdits.empty())
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No replacements");
+ auto Edits = Effect->ApplyEdits;
+ if (Edits.size() > 1)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Multi-file edits");
Annotations Code(Input);
- return applyAllReplacements(Code.code(), *Effect->ApplyEdit);
+ return applyAllReplacements(Code.code(), Edits.begin()->second.Replacements);
}
void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -9,8 +9,8 @@
#include "TweakTesting.h"
#include "Annotations.h"
-#include "refactor/Tweak.h"
#include "SourceCode.h"
+#include "refactor/Tweak.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/Support/Error.h"
@@ -98,14 +98,16 @@
return "fail: " + llvm::toString(Result.takeError());
if (Result->ShowMessage)
return "message:\n" + *Result->ShowMessage;
- if (Result->ApplyEdit) {
-if (auto NewText =
-tooling::applyAllReplacements(Input.code(), *Result->ApplyEdit))
- return unwrap(Context, *NewText);
-else
- return "bad edits: " + llvm::toString(NewText.takeError());
- }
- return "no effect";
+ if (Result->ApplyEdits.empty())
+return "no effect";
+ if (Result->ApplyEdits.size() > 1)
+return "received multi-file edits";
+
+ auto ApplyEdit = Result->ApplyEdits.begin()->second;
+ if (auto NewText = ApplyEdit.apply())
+return unwrap(Context, *NewText);
+ else
+return "bad edits: " + llvm::toString(NewText.takeError());
}
::testing::Matcher TweakTest::isAvailable() const {
Index: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -90,7 +90,7 @@
ElseRng->getBegin(),
ElseCode.size(), ThenCode)))
return std::move(Err);
- return Effect::applyEdit(Result);
+ return mainFileEdit(SrcMgr, std::move(Result));
}
} // namespace
Index: clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
@@ -88,10 +88,12 @@
}
Expected RawStringLiteral::apply(const Selection ) {
- return Effect::applyEdit(tooling::Replacements(
+ auto = Inputs.AST.getSourceManager();
+ auto Reps = tooling::Replacements(
tooling::Replacement(Inputs.AST.getSourceManager(), Str,
("R\"(" + Str->getBytes() + ")\"").str(),
- Inputs.AST.getASTContext().getLangOpts(;
+ Inputs.AST.getASTContext().getLangOpts()));
+ return mainFileEdit(SM, std::move(Reps));
}
} // namespace
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -468,7 +468,7 @@
// replace
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet marked 14 inline comments as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/SourceCode.cpp:601 + llvm::inconvertibleErrorCode(), + "File contents differ on disk for %s, please save", FilePath.data()); +} sammccall wrote: > you seem to be checking this both here and in clangdlspserver. Why? the contents in here are coming from disk, whereas the one in clangdlspserver is coming from drafts. I suppose it would make more sense to merge the two in clangdlspserver, use draft manager as content source backed by disk in case user hasn't opened that file. Comment at: clang-tools-extra/clangd/SourceCode.h:215 +/// Formats the edits in \p ApplyEdits and generates TextEdits from those. +/// Ensures the files in FS are consistent with the files used while generating sammccall wrote: > not sure what "generates TextEdits from those" refers to. > > Could this function be called "reformatEdits" or "formatAroundEdits"? > not sure what "generates TextEdits from those" refers to. Leftover from an old-version. Comment at: clang-tools-extra/clangd/SourceCode.h:220 +llvm::Error formatAndGenerateEdits(llvm::vfs::FileSystem *FS, + llvm::StringMap , + llvm::StringRef MainFilePath, sammccall wrote: > This signature is confusing: we pass code in *three* different ways (FS, > Edits, and MainFileCode). Much of this is because we put the loop (and > therefore all special cases) inside this function. > The logic around the way the VFS is mapped for the main file vs others > doesn't really belong in this layer. Neither does "please save"... > > It seems this wants to be something simpler/smaller like `reformatEdit(const > Edit&, const Style&)` that could be called from ClangdServer. There's > probably another helper like `checkApplies(const Edit&, VFS*)` that would go > in ClangdLSPServer. > as discussed offline for `reformatEdit` to work, made `InitialCode` in `Edit` public. As both formatting and style deduction requires access to file contents. for `checkApplies`, it requires access to both `VFS` and `DraftMgr`and pretty special use-case for clangdlspserver, so don't think the function would carry its weight. Therefore left the check inlined. Comment at: clang-tools-extra/clangd/refactor/Tweak.h:74 +/// A mapping from absolute file path to edits. +llvm::Optional> ApplyEdits; sammccall wrote: > what's the difference between `None` and an empty map? `None` :P Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66637/new/ https://reviews.llvm.org/D66637 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/SourceCode.cpp:601
+ llvm::inconvertibleErrorCode(),
+ "File contents differ on disk for %s, please save", FilePath.data());
+}
you seem to be checking this both here and in clangdlspserver. Why?
Comment at: clang-tools-extra/clangd/SourceCode.h:42
+/// Represents a set of edits generated for a single file.
+struct Edit {
nit: drop "represents"
Comment at: clang-tools-extra/clangd/SourceCode.h:42
+/// Represents a set of edits generated for a single file.
+struct Edit {
sammccall wrote:
> nit: drop "represents"
nit: this could also describe Replacements, vector, etc. Motivate
this class a little more?
Comment at: clang-tools-extra/clangd/SourceCode.h:215
+/// Formats the edits in \p ApplyEdits and generates TextEdits from those.
+/// Ensures the files in FS are consistent with the files used while generating
not sure what "generates TextEdits from those" refers to.
Could this function be called "reformatEdits" or "formatAroundEdits"?
Comment at: clang-tools-extra/clangd/SourceCode.h:220
+llvm::Error formatAndGenerateEdits(llvm::vfs::FileSystem *FS,
+ llvm::StringMap ,
+ llvm::StringRef MainFilePath,
This signature is confusing: we pass code in *three* different ways (FS, Edits,
and MainFileCode). Much of this is because we put the loop (and therefore all
special cases) inside this function.
The logic around the way the VFS is mapped for the main file vs others doesn't
really belong in this layer. Neither does "please save"...
It seems this wants to be something simpler/smaller like `reformatEdit(const
Edit&, const Style&)` that could be called from ClangdServer. There's probably
another helper like `checkApplies(const Edit&, VFS*)` that would go in
ClangdLSPServer.
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:74
+/// A mapping from absolute file path to edits.
+llvm::Optional> ApplyEdits;
what's the difference between `None` and an empty map?
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:76
-static Effect applyEdit(tooling::Replacements R) {
+void addEdit(StringRef FilePath, Edit Ed) {
+ assert(ApplyEdits);
(if the null map case goes away, this can too)
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:81
+
+static Effect applyEdit(StringRef FilePath, Edit Ed) {
Effect E;
This greatly increases the burden of callers of this function, who mostly want
to edit the current file.
- need to provide a fully-qualified name (I don't think the implementations in
this patch actually do that)
- need to construct an Edit
can we provide an `Effect mainFileEdit(const SourceManager&, Replacements)`?
and maybe `Effect fileEdit(const SourceManager&, FileID, Replacements)` though
maybe the latter doesn't cover enough cases to pull its weight.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet updated this revision to Diff 216823.
kadircet marked 8 inline comments as done.
kadircet added a comment.
- Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -113,11 +113,15 @@
auto Effect = apply(ID, Input);
if (!Effect)
return Effect.takeError();
- if (!Effect->ApplyEdit)
+ if (!Effect->ApplyEdits)
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No replacements");
+ auto Edits = *Effect->ApplyEdits;
+ if (Edits.size() > 1)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Multi-file edits");
Annotations Code(Input);
- return applyAllReplacements(Code.code(), *Effect->ApplyEdit);
+ return applyAllReplacements(Code.code(), Edits.begin()->second.Replacements);
}
void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -9,8 +9,8 @@
#include "TweakTesting.h"
#include "Annotations.h"
-#include "refactor/Tweak.h"
#include "SourceCode.h"
+#include "refactor/Tweak.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/Support/Error.h"
@@ -98,9 +98,11 @@
return "fail: " + llvm::toString(Result.takeError());
if (Result->ShowMessage)
return "message:\n" + *Result->ShowMessage;
- if (Result->ApplyEdit) {
-if (auto NewText =
-tooling::applyAllReplacements(Input.code(), *Result->ApplyEdit))
+ if (Result->ApplyEdits) {
+if (Result->ApplyEdits->size() > 1)
+ return "received multi-file edits";
+auto ApplyEdit = Result->ApplyEdits->begin()->second;
+if (auto NewText = ApplyEdit.apply())
return unwrap(Context, *NewText);
else
return "bad edits: " + llvm::toString(NewText.takeError());
Index: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -90,7 +90,8 @@
ElseRng->getBegin(),
ElseCode.size(), ThenCode)))
return std::move(Err);
- return Effect::applyEdit(Result);
+ return Effect::applyEdit(SrcMgr.getFilename(Inputs.Cursor),
+ Edit(Inputs.Code, std::move(Result)));
}
} // namespace
Index: clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
@@ -88,10 +88,13 @@
}
Expected RawStringLiteral::apply(const Selection ) {
- return Effect::applyEdit(tooling::Replacements(
+ auto = Inputs.AST.getSourceManager();
+ auto Reps = tooling::Replacements(
tooling::Replacement(Inputs.AST.getSourceManager(), Str,
("R\"(" + Str->getBytes() + ")\"").str(),
- Inputs.AST.getASTContext().getLangOpts(;
+ Inputs.AST.getASTContext().getLangOpts()));
+ return Effect::applyEdit(SM.getFilename(Inputs.Cursor),
+ Edit(Inputs.Code, std::move(Reps)));
}
} // namespace
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -468,7 +468,9 @@
// replace expression with variable name
if (auto Err = Result.add(Target->replaceWithVar(Range, VarName)))
return
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:644
+ assert(It.second.Edits && "TextEdits hasn't been generated?");
+ if (auto Draft = DraftMgr.getDraft(It.first())) {
+llvm::StringRef Contents = *Draft;
kadircet wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > This callback is called asynchronously and the version inside `DraftMgr`
> > > may be newer than the one we used to calculate the original offsets
> > > inside replacements at this point.
> > >
> > > We should not rely on `DraftMgr`, doing conversions on the source buffers
> > > inside `SourceManager` is probably the most reliable option we have.
> > I think you've misunderstood what this code is doing (it needs comments!)
> >
> > The SourceManager contains the code that we've calculated the edits
> > against, by definition. So checking against it does nothing.
> >
> > When we send the edits to the client, it's going to apply them to the file
> > if it's not open, but to the dirty buffer if it is open.
> > If the dirty buffer has different contents than what we saw (and calculated
> > edits against) then the edits will be wrong - that's what this code guards
> > against.
> >
> > So looking at the latest available content in the draftmgr (at the end of
> > the request, not the start!) is the right thing here, I think.
> > This callback is called asynchronously and the version inside DraftMgr may
> > be newer than the one we used to calculate the original offsets inside
> > replacements at this point.
>
> That's exactly what we are checking though?
>
> We want to bail out if user has a "different" version of the source code in
> their editor by the time we generated edits. Since editors will most likely
> try to apply the edits onto the version in editor.
You are right, I'm sorry. I did not read into the code, just assumed it was
doing something it doesn't.
Again, very sorry for the disturbance.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/SourceCode.h:41
+/// Represents a set of edits generated for a single file.
+struct Edit {
+ /// SHA1 hash of the file contents for the edits generated below. Clients
Hmm, it is also attractive to make this something like
```
struct Edit {
string CodeBefore;
tooling::Replacements Reps;
string codeAfter();
std::vector asTextEdits();
bool canApplyTo(StringRef CodeBefore); // does line-ending normalization
}
```
if we were worried about abuse, we could make CodeBefore private
Comment at: clang-tools-extra/clangd/SourceCode.h:43
+ /// SHA1 hash of the file contents for the edits generated below. Clients
+ /// should verify contents they see are the same as this one before applying
+ /// edits.
, if possible.
(Sometimes we're just sending these over the wire)
Comment at: clang-tools-extra/clangd/SourceCode.h:46
+ std::array ContentDigests;
+ tooling::Replacements Reps;
+ llvm::Optional> Edits;
document that these are equivalent.
Why is Edits optional?
Comment at: clang-tools-extra/clangd/SourceCode.h:46
+ std::array ContentDigests;
+ tooling::Replacements Reps;
+ llvm::Optional> Edits;
sammccall wrote:
> document that these are equivalent.
> Why is Edits optional?
nit: reps -> replacements
ContentDigests -> ContentDigest
Comment at: clang-tools-extra/clangd/SourceCode.h:49
+
+ static Edit generateEdit(llvm::StringRef Code, tooling::Replacements Reps) {
+Edit E;
make this a constructor?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:644
+ assert(It.second.Edits && "TextEdits hasn't been generated?");
+ if (auto Draft = DraftMgr.getDraft(It.first())) {
+llvm::StringRef Contents = *Draft;
ilya-biryukov wrote:
> This callback is called asynchronously and the version inside `DraftMgr` may
> be newer than the one we used to calculate the original offsets inside
> replacements at this point.
>
> We should not rely on `DraftMgr`, doing conversions on the source buffers
> inside `SourceManager` is probably the most reliable option we have.
I think you've misunderstood what this code is doing (it needs comments!)
The SourceManager contains the code that we've calculated the edits against, by
definition. So checking against it does nothing.
When we send the edits to the client, it's going to apply them to the file if
it's not open, but to the dirty buffer if it is open.
If the dirty buffer has different contents than what we saw (and calculated
edits against) then the edits will be wrong - that's what this code guards
against.
So looking at the latest available content in the draftmgr (at the end of the
request, not the start!) is the right thing here, I think.
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:646
+llvm::StringRef Contents = *Draft;
+if (llvm::SHA1::hash(llvm::makeArrayRef(Contents.bytes_begin(),
+Contents.size())) !=
We're comparing here content in the source manager (which was read as bytes
from disk, and converted to utf8) to that from LSP (which was sent as unicode
text by the editor, and converted to utf8).
Are editors going to preserve the actual line endings from disk when sending
LSP, or are they going to use the "array-of-lines" from their editor's native
model? (e.g. join(lines, '\n'))
If the latter, we'd need to normalize line endings, possibly strip trailing
line endings, etc.
I ask because clangd-vim definitely has an "array-of-lines" model :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet marked an inline comment as done.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:644
+ assert(It.second.Edits && "TextEdits hasn't been generated?");
+ if (auto Draft = DraftMgr.getDraft(It.first())) {
+llvm::StringRef Contents = *Draft;
sammccall wrote:
> ilya-biryukov wrote:
> > This callback is called asynchronously and the version inside `DraftMgr`
> > may be newer than the one we used to calculate the original offsets inside
> > replacements at this point.
> >
> > We should not rely on `DraftMgr`, doing conversions on the source buffers
> > inside `SourceManager` is probably the most reliable option we have.
> I think you've misunderstood what this code is doing (it needs comments!)
>
> The SourceManager contains the code that we've calculated the edits against,
> by definition. So checking against it does nothing.
>
> When we send the edits to the client, it's going to apply them to the file if
> it's not open, but to the dirty buffer if it is open.
> If the dirty buffer has different contents than what we saw (and calculated
> edits against) then the edits will be wrong - that's what this code guards
> against.
>
> So looking at the latest available content in the draftmgr (at the end of the
> request, not the start!) is the right thing here, I think.
> This callback is called asynchronously and the version inside DraftMgr may be
> newer than the one we used to calculate the original offsets inside
> replacements at this point.
That's exactly what we are checking though?
We want to bail out if user has a "different" version of the source code in
their editor by the time we generated edits. Since editors will most likely try
to apply the edits onto the version in editor.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
ilya-biryukov added inline comments.
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:644
+ assert(It.second.Edits && "TextEdits hasn't been generated?");
+ if (auto Draft = DraftMgr.getDraft(It.first())) {
+llvm::StringRef Contents = *Draft;
This callback is called asynchronously and the version inside `DraftMgr` may be
newer than the one we used to calculate the original offsets inside
replacements at this point.
We should not rely on `DraftMgr`, doing conversions on the source buffers
inside `SourceManager` is probably the most reliable option we have.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks
kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
First patch for propogating multifile changes from tweak outputs to LSP
WorkspaceEdits.
Uses FS to convert tooling::Replacements to TextEdits except the MainFile.
Errors out if there are any inconsistencies between the draft version and the
version generated the edits.
We exempt MainFile from this check to not regress existing use cases, since
clangd can refactor the mainfile even if there are unsaved changes in the
editor. But for other files it will make use of on-disk state rather than
on-editor state.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D66637
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -113,11 +113,15 @@
auto Effect = apply(ID, Input);
if (!Effect)
return Effect.takeError();
- if (!Effect->ApplyEdit)
+ if (!Effect->ApplyEdits)
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No replacements");
+ auto Edits = *Effect->ApplyEdits;
+ if (Edits.size() > 1)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Multi-file edits");
Annotations Code(Input);
- return applyAllReplacements(Code.code(), *Effect->ApplyEdit);
+ return applyAllReplacements(Code.code(), Edits.begin()->second.Reps);
}
void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -9,8 +9,8 @@
#include "TweakTesting.h"
#include "Annotations.h"
-#include "refactor/Tweak.h"
#include "SourceCode.h"
+#include "refactor/Tweak.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/Support/Error.h"
@@ -98,9 +98,12 @@
return "fail: " + llvm::toString(Result.takeError());
if (Result->ShowMessage)
return "message:\n" + *Result->ShowMessage;
- if (Result->ApplyEdit) {
+ if (Result->ApplyEdits) {
+if (Result->ApplyEdits->size() > 1)
+ return "received multi-file edits";
+auto ApplyEdit = Result->ApplyEdits->begin()->second;
if (auto NewText =
-tooling::applyAllReplacements(Input.code(), *Result->ApplyEdit))
+tooling::applyAllReplacements(Input.code(), ApplyEdit.Reps))
return unwrap(Context, *NewText);
else
return "bad edits: " + llvm::toString(NewText.takeError());
Index: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -90,7 +90,8 @@
ElseRng->getBegin(),
ElseCode.size(), ThenCode)))
return std::move(Err);
- return Effect::applyEdit(Result);
+ return Effect::applyEdit(SrcMgr.getFilename(Inputs.Cursor),
+ Edit::generateEdit(Inputs.Code, std::move(Result)));
}
} // namespace
Index: clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp
@@ -88,10 +88,13 @@
}
Expected RawStringLiteral::apply(const Selection ) {
- return Effect::applyEdit(tooling::Replacements(
+ auto = Inputs.AST.getSourceManager();
+ auto Reps = tooling::Replacements(
tooling::Replacement(Inputs.AST.getSourceManager(), Str,
("R\"(" + Str->getBytes() + ")\"").str(),
- Inputs.AST.getASTContext().getLangOpts(;
+ Inputs.AST.getASTContext().getLangOpts()));
+ return
