kadircet created this revision.
kadircet added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
kadircet added a parent revision: D69298: [clangd] Define out-of-line initial
apply logic.
Return type might need qualification if insertion context doesn't have
the same decls visible as the source context.
This patch adds qualification for return value to cover such cases.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D70535
Files:
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.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
@@ -1761,6 +1761,57 @@
"template <> void foo<int>() { return; }")));
}
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+ FileName = "Test.hpp";
+ ExtraFiles["Test.cpp"] = "";
+
+ struct {
+ llvm::StringRef Test;
+ llvm::StringRef ExpectedHeader;
+ llvm::StringRef ExpectedSource;
+ } Cases[] = {
+ {R"cpp(
+ namespace a { class Foo; }
+ using namespace a;
+ Foo fo^o() { return; })cpp",
+ R"cpp(
+ namespace a { class Foo; }
+ using namespace a;
+ Foo foo() ;)cpp",
+ "a::Foo foo() { return; }"},
+ {R"cpp(
+ namespace a {
+ class Foo {
+ class Bar {};
+ Bar fo^o() { return {}; }
+ }
+ })cpp",
+ R"cpp(
+ namespace a {
+ class Foo {
+ class Bar {};
+ Bar foo() ;
+ }
+ })cpp",
+ "a::Foo::Bar foo() { return {}; }\n"},
+ {R"cpp(
+ class Foo;
+ Foo fo^o() { return; })cpp",
+ R"cpp(
+ class Foo;
+ Foo foo() ;)cpp",
+ "Foo foo() { return; }"},
+ };
+ llvm::StringMap<std::string> EditedFiles;
+ for (auto &Case : Cases) {
+ EditedFiles.clear();
+ apply(Case.Test, &EditedFiles);
+ EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+ llvm::errs() << EditedFiles.begin()->second << '\n';
+ EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+ testPath("Test.cpp"), Case.ExpectedSource)));
+ }
+}
} // namespace
} // namespace clangd
} // namespace clang
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
@@ -6,13 +6,17 @@
//
//===----------------------------------------------------------------------===//
+#include "AST.h"
+#include "FindTarget.h"
#include "HeaderSourceSwitch.h"
+#include "Logger.h"
#include "Path.h"
#include "Selection.h"
#include "SourceCode.h"
#include "refactor/Tweak.h"
#include "clang/AST/ASTTypeTraits.h"
#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Stmt.h"
#include "clang/Basic/SourceLocation.h"
@@ -22,8 +26,10 @@
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
#include "llvm/Support/Error.h"
#include <cstddef>
+#include <string>
namespace clang {
namespace clangd {
@@ -70,27 +76,89 @@
// 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) {
+llvm::Expected<std::string>
+getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace) {
auto &SM = FD->getASTContext().getSourceManager();
- auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
- FD->getSourceRange());
- if (!CharRange)
- return llvm::None;
- CharRange->setBegin(getBeginLoc(FD));
+ const DeclContext *TargetContext = FD->getLexicalDeclContext();
+ while (!TargetContext->isTranslationUnit()) {
+ if (TargetContext->isNamespace()) {
+ auto *NSD = llvm::dyn_cast<NamespaceDecl>(TargetContext);
+ if (NSD->getNameAsString() == TargetNamespace)
+ break;
+ }
+ TargetContext = TargetContext->getLexicalParent();
+ }
+
+ bool HadErrors = false;
+ tooling::Replacements Replacements;
+ findExplicitReferences(FD, [&](ReferenceLoc Ref) {
+ // It is enough to qualify the first qualifier, so skip references with a
+ // qualifier. Also we can't do much if there are no targets or name is
+ // inside a macro body.
+ if (Ref.Qualifier || Ref.Targets.empty() || Ref.NameLoc.isMacroID())
+ return;
+ // Qualify return type
+ if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin())
+ return;
+
+ for (const NamedDecl *ND : Ref.Targets) {
+ if (ND->getDeclContext() != Ref.Targets.front()->getDeclContext()) {
+ elog("Targets from multiple contexts: {0}, {1}",
+ printQualifiedName(*Ref.Targets.front()), printQualifiedName(*ND));
+ return;
+ }
+ }
+ const NamedDecl *ND = Ref.Targets.front();
+ const std::string Qualifier =
+ getQualification(FD->getASTContext(), TargetContext,
+ SM.getLocForStartOfFile(SM.getMainFileID()), ND);
+ if (auto Err = Replacements.add(
+ tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) {
+ elog("define outline: Failed to add quals: {0}", std::move(Err));
+ HadErrors = true;
+ }
+ });
+
+ if (HadErrors) {
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "define outline: Failed to compute qualifiers see logs for details.");
+ }
+
+ // Get new begin and end positions for the qualified body.
+ auto OrigFuncRange = toHalfOpenFileRange(
+ SM, FD->getASTContext().getLangOpts(), FD->getSourceRange());
+ if (!OrigFuncRange)
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Couldn't get range for function.");
+ OrigFuncRange->setBegin(getBeginLoc(FD));
+
+ unsigned FuncBegin = SM.getFileOffset(OrigFuncRange->getBegin());
+ unsigned FuncEnd = Replacements.getShiftedCodePosition(
+ SM.getFileOffset(OrigFuncRange->getEnd()));
+
+ // Trim the result to function body.
+ auto QualifiedFunc = tooling::applyAllReplacements(
+ SM.getBufferData(SM.getMainFileID()), Replacements);
+ if (!QualifiedFunc)
+ return QualifiedFunc.takeError();
+ return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
- // FIXME: Qualify return type.
// FIXME: Qualify function name depending on the target context.
- return toSourceCode(SM, *CharRange);
}
+struct InsertionPoint {
+ std::string EnclosingNamespace;
+ size_t Offset;
+};
// 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) {
+llvm::Expected<InsertionPoint>
+getInsertionPoint(llvm::StringRef Contents, llvm::StringRef QualifiedName,
+ const format::FormatStyle &Style) {
auto Region = getEligiblePoints(Contents, QualifiedName, Style);
assert(!Region.EligiblePoints.empty());
@@ -98,8 +166,10 @@
// 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);
+ auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
+ if (!Offset)
+ return Offset.takeError();
+ return InsertionPoint{Region.EnclosingNamespace, *Offset};
}
/// Moves definition of a function/method to an appropriate implementation file.
@@ -171,21 +241,22 @@
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 InsertionPoint =
+ getInsertionPoint(Contents, Source->getQualifiedNameAsString(),
+ getFormatStyleForFile(*CCFile, Contents, &FS));
+ if (!InsertionPoint)
+ return InsertionPoint.takeError();
- auto FuncDef = getFunctionSourceCode(Source);
+ auto FuncDef =
+ getFunctionSourceCode(Source, InsertionPoint->EnclosingNamespace);
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);
+ const tooling::Replacement InsertFunctionDef(
+ *CCFile, InsertionPoint->Offset, 0, *FuncDef);
auto Effect = Effect::mainFileEdit(
SMFF.get(), tooling::Replacements(InsertFunctionDef));
if (!Effect)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
