njames93 updated this revision to Diff 513911.
njames93 added a comment.
- Clang format changes
- Update apply logic to not query the FS when its not a cross file edit
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148423/new/
https://reviews.llvm.org/D148423
Files:
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -120,6 +120,22 @@
EXPECT_EQ(apply(Test), Expected);
}
+TEST_F(DefineOutlineTest, SourceFileAllowed) {
+ FileName = "Test.cpp";
+
+ llvm::StringRef Test = "struct A { A^() {} };\n";
+ llvm::StringRef Expected = "struct A { A() ; };\nA::A() {} ";
+ EXPECT_EQ(Expected, apply(Test));
+
+ Test = "struct A { struct B { B^() {} }; };\n";
+ Expected = "struct A { struct B { B() ; }; };\nA::B::B() {} ";
+ EXPECT_EQ(Expected, apply(Test));
+
+ Test = "struct A { struct B; };\nstruct A::B { B^() {} };\n";
+ Expected = "struct A { struct B; };\nstruct A::B { B() ; };\nA::B::B() {} ";
+ EXPECT_EQ(Expected, apply(Test));
+}
+
TEST_F(DefineOutlineTest, ApplyTest) {
llvm::StringMap<std::string> EditedFiles;
ExtraFiles["Test.cpp"] = "";
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -386,12 +386,9 @@
}
bool prepare(const Selection &Sel) override {
- // Bail out if we are not in a header file.
- // FIXME: We might want to consider moving method definitions below class
- // definition even if we are inside a source file.
- if (!isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor),
- Sel.AST->getLangOpts()))
- return false;
+ bool IsHeader =
+ isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor),
+ Sel.AST->getLangOpts());
Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
// Bail out if the selection is not a in-line function definition.
@@ -399,8 +396,15 @@
Source->isOutOfLine())
return false;
+ // If we are in a source file and the function is a free function, bail out.
+ if (!isa<CXXMethodDecl>(Source) && !IsHeader)
+ return false;
+
+ IsCrossFileEdit = IsHeader;
+
// Bail out if this is a function template or specialization, as their
// definitions need to be visible in all including translation units.
+ // FIXME: This can be supported if we are not making a cross file edit.
if (Source->getDescribedFunctionTemplate())
return false;
if (Source->getTemplateSpecializationInfo())
@@ -430,19 +434,33 @@
Expected<Effect> apply(const Selection &Sel) override {
const SourceManager &SM = Sel.AST->getSourceManager();
- auto CCFile = getSourceFile(Sel.AST->tuPath(), Sel);
-
- if (!CCFile)
- return error("Couldn't find a suitable implementation file.");
- assert(Sel.FS && "FS Must be set in apply");
- auto Buffer = Sel.FS->getBufferForFile(*CCFile);
- // FIXME: Maybe we should consider creating the implementation file if it
- // doesn't exist?
- if (!Buffer)
- return llvm::errorCodeToError(Buffer.getError());
- auto Contents = Buffer->get()->getBuffer();
- auto InsertionPoint = getInsertionPoint(
- Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
+
+ std::optional<Path> OwnedPath;
+ std::unique_ptr<llvm::MemoryBuffer> OwnedBuffer;
+ PathRef Path;
+ StringRef TargetContents;
+ if (IsCrossFileEdit) {
+ OwnedPath = getSourceFile(Sel.AST->tuPath(), Sel);
+
+ if (!OwnedPath)
+ return error("Couldn't find a suitable implementation file.");
+ Path = *OwnedPath;
+ assert(Sel.FS && "FS Must be set in apply");
+ if (auto Buffer = Sel.FS->getBufferForFile(Path))
+ OwnedBuffer = std::move(Buffer.get());
+ else
+ // FIXME: Maybe we should consider creating the implementation file if
+ // it doesn't exist?
+ return llvm::errorCodeToError(Buffer.getError());
+ TargetContents = OwnedBuffer->getBuffer();
+ } else {
+ Path = Sel.AST->tuPath();
+ TargetContents = Sel.Code;
+ }
+
+ auto InsertionPoint =
+ getInsertionPoint(TargetContents, Source->getQualifiedNameAsString(),
+ Sel.AST->getLangOpts());
if (!InsertionPoint)
return InsertionPoint.takeError();
@@ -452,14 +470,6 @@
if (!FuncDef)
return FuncDef.takeError();
- SourceManagerForFile SMFF(*CCFile, Contents);
- const tooling::Replacement InsertFunctionDef(
- *CCFile, InsertionPoint->Offset, 0, *FuncDef);
- auto Effect = Effect::mainFileEdit(
- SMFF.get(), tooling::Replacements(InsertFunctionDef));
- if (!Effect)
- return Effect.takeError();
-
// FIXME: We should also get rid of inline qualifier.
const tooling::Replacement DeleteFuncBody(
Sel.AST->getSourceManager(),
@@ -467,18 +477,35 @@
SM, Sel.AST->getLangOpts(),
getDeletionRange(Source, Sel.AST->getTokens()))),
";");
- auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(),
- tooling::Replacements(DeleteFuncBody));
- if (!HeaderFE)
- return HeaderFE.takeError();
-
- Effect->ApplyEdits.try_emplace(HeaderFE->first,
- std::move(HeaderFE->second));
- return std::move(*Effect);
+ const tooling::Replacement InsertFunctionDef(Path, InsertionPoint->Offset,
+ 0, *FuncDef);
+
+ if (IsCrossFileEdit) {
+ SourceManagerForFile SMFF(Path, TargetContents);
+ auto Effect = Effect::mainFileEdit(
+ SMFF.get(), tooling::Replacements(InsertFunctionDef));
+ if (!Effect)
+ return Effect.takeError();
+
+ auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(),
+ tooling::Replacements(DeleteFuncBody));
+ if (!HeaderFE)
+ return HeaderFE.takeError();
+
+ Effect->ApplyEdits.try_emplace(HeaderFE->first,
+ std::move(HeaderFE->second));
+ return std::move(*Effect);
+ }
+ tooling::Replacements R(DeleteFuncBody);
+ if (auto Err = R.add(InsertFunctionDef)) {
+ return Err;
+ }
+ return Effect::mainFileEdit(SM, R);
}
private:
const FunctionDecl *Source = nullptr;
+ bool IsCrossFileEdit = false;
};
REGISTER_TWEAK(DefineOutline)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
