kadircet updated this revision to Diff 231382.
kadircet marked an inline comment as done.
kadircet added a comment.
- Address comments
- Better handling for macros and tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69298/new/
https://reviews.llvm.org/D69298
Files:
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.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
@@ -1870,6 +1870,110 @@
})cpp");
}
+TEST_F(DefineOutlineTest, FailsWithoutSource) {
+ FileName = "Test.hpp";
+ llvm::StringRef Test = "void fo^o() { return; }";
+ llvm::StringRef Expected =
+ "fail: Couldn't find a suitable implementation file.";
+ EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DefineOutlineTest, ApplyTest) {
+ llvm::StringMap<std::string> EditedFiles;
+ ExtraFiles["Test.cpp"] = "";
+ FileName = "Test.hpp";
+
+ struct {
+ llvm::StringRef Test;
+ llvm::StringRef ExpectedHeader;
+ llvm::StringRef ExpectedSource;
+ } Cases[] = {
+ // Simple check
+ {
+ "void fo^o() { return; }",
+ "void foo() ;",
+ "void foo() { return; }",
+ },
+ // Templated function.
+ {
+ "template <typename T> void fo^o(T, T x) { return; }",
+ "template <typename T> void foo(T, T x) ;",
+ "template <typename T> void foo(T, T x) { return; }",
+ },
+ {
+ "template <typename> void fo^o() { return; }",
+ "template <typename> void foo() ;",
+ "template <typename> void foo() { return; }",
+ },
+ // Template specialization.
+ {
+ R"cpp(
+ template <typename> void foo();
+ template <> void fo^o<int>() { return; })cpp",
+ R"cpp(
+ template <typename> void foo();
+ template <> void foo<int>() ;)cpp",
+ "template <> void foo<int>() { return; }",
+ },
+ };
+ for (const auto &Case : Cases) {
+ SCOPED_TRACE(Case.Test);
+ EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+ EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)));
+ }
+}
+
+TEST_F(DefineOutlineTest, HandleMacros) {
+ llvm::StringMap<std::string> EditedFiles;
+ ExtraFiles["Test.cpp"] = "";
+ FileName = "Test.hpp";
+
+ struct {
+ llvm::StringRef Test;
+ llvm::StringRef ExpectedHeader;
+ llvm::StringRef ExpectedSource;
+ } Cases[] = {
+ {R"cpp(
+ #define BODY { return; }
+ void f^oo()BODY)cpp",
+ R"cpp(
+ #define BODY { return; }
+ void foo();)cpp",
+ "void foo()BODY"},
+
+ {R"cpp(
+ #define BODY return;
+ void f^oo(){BODY})cpp",
+ R"cpp(
+ #define BODY return;
+ void foo();)cpp",
+ "void foo(){BODY}"},
+
+ {R"cpp(
+ #define TARGET void foo()
+ [[TARGET]]{ return; })cpp",
+ R"cpp(
+ #define TARGET void foo()
+ TARGET;)cpp",
+ "TARGET{ return; }"},
+
+ {R"cpp(
+ #define TARGET foo
+ void [[TARGET]](){ return; })cpp",
+ R"cpp(
+ #define TARGET foo
+ void TARGET();)cpp",
+ "void TARGET(){ return; }"},
+ };
+ for (const auto &Case : Cases) {
+ SCOPED_TRACE(Case.Test);
+ EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+ EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)));
+ }
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -127,7 +127,7 @@
ADD_FAILURE() << "There were changes to additional files, but client "
"provided a nullptr for EditedFiles.";
else
- EditedFiles->try_emplace(It.first(), Unwrapped.str());
+ EditedFiles->insert_or_assign(It.first(), Unwrapped.str());
}
}
return EditedMainFile;
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
@@ -13,9 +13,17 @@
#include "refactor/Tweak.h"
#include "clang/AST/ASTTypeTraits.h"
#include "clang/AST/Decl.h"
+#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Stmt.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Driver/Types.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
#include "llvm/ADT/Optional.h"
-#include "llvm/Support/Path.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include <cstddef>
namespace clang {
namespace clangd {
@@ -40,6 +48,51 @@
return nullptr;
}
+llvm::Optional<Path> getSourceFile(llvm::StringRef FileName,
+ const Tweak::Selection &Sel) {
+ if (auto Source = getCorrespondingHeaderOrSource(
+ FileName,
+ &Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem()))
+ return *Source;
+ return getCorrespondingHeaderOrSource(FileName, Sel.AST, Sel.Index);
+}
+
+// Creates a modified version of function definition that can be inserted at a
+// different location. Contains both function signature and body.
+llvm::Optional<llvm::StringRef> getFunctionSourceCode(const FunctionDecl *FD) {
+ auto &SM = FD->getASTContext().getSourceManager();
+ auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
+ FD->getSourceRange());
+ if (!CharRange)
+ return llvm::None;
+ // Include template parameter list.
+ if (auto *FTD = FD->getDescribedFunctionTemplate())
+ CharRange->setBegin(FTD->getBeginLoc());
+
+ // FIXME: Qualify return type.
+ // FIXME: Qualify function name depending on the target context.
+ return toSourceCode(SM, *CharRange);
+}
+
+// Returns the most natural insertion point for \p QualifiedName in \p Contents.
+// This currently cares about only the namespace proximity, but in feature it
+// should also try to follow ordering of declarations. For example, if decls
+// come in order `foo, bar, baz` then this function should return some point
+// between foo and baz for inserting bar.
+llvm::Expected<size_t> getInsertionOffset(llvm::StringRef Contents,
+ llvm::StringRef QualifiedName,
+ const format::FormatStyle &Style) {
+ auto Region = getEligiblePoints(Contents, QualifiedName, Style);
+
+ assert(!Region.EligiblePoints.empty());
+ // FIXME: This selection can be made smarter by looking at the definition
+ // locations for adjacent decls to Source. Unfortunately psudeo parsing in
+ // getEligibleRegions only knows about namespace begin/end events so we
+ // can't match function start/end positions yet.
+ auto InsertionPoint = Region.EligiblePoints.back();
+ return positionToOffset(Contents, InsertionPoint);
+}
+
/// Moves definition of a function/method to an appropriate implementation file.
///
/// Before:
@@ -94,8 +147,64 @@
}
Expected<Effect> apply(const Selection &Sel) override {
- return llvm::createStringError(llvm::inconvertibleErrorCode(),
- "Not implemented yet");
+ const SourceManager &SM = Sel.AST.getSourceManager();
+ auto MainFileName =
+ getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
+ if (!MainFileName)
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "Couldn't get absolute path for mainfile.");
+
+ auto CCFile = getSourceFile(*MainFileName, Sel);
+ if (!CCFile)
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "Couldn't find a suitable implementation file.");
+
+ auto &FS =
+ Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem();
+ auto Buffer = FS.getBufferForFile(*CCFile);
+ // FIXME: Maybe we should consider creating the implementation file if it
+ // doesn't exist?
+ if (!Buffer)
+ return llvm::createStringError(Buffer.getError(),
+ Buffer.getError().message());
+ auto Contents = Buffer->get()->getBuffer();
+ auto InsertionOffset =
+ getInsertionOffset(Contents, Source->getQualifiedNameAsString(),
+ getFormatStyleForFile(*CCFile, Contents, &FS));
+ if (!InsertionOffset)
+ return InsertionOffset.takeError();
+
+ auto FuncDef = getFunctionSourceCode(Source);
+ if (!FuncDef)
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "Couldn't get full source for function definition.");
+
+ SourceManagerForFile SMFF(*CCFile, Contents);
+ const tooling::Replacement InsertFunctionDef(*CCFile, *InsertionOffset, 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(),
+ CharSourceRange::getTokenRange(
+ *toHalfOpenFileRange(SM, Sel.AST.getASTContext().getLangOpts(),
+ Source->getBody()->getSourceRange())),
+ ";");
+ 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);
}
private:
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
