kadircet updated this revision to Diff 221947.
kadircet added a comment.
- Use canonical decl when checking visibility
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65433/new/
https://reviews.llvm.org/D65433
Files:
clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTesting.h
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
@@ -24,6 +24,7 @@
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/MemoryBuffer.h"
@@ -33,6 +34,8 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <cassert>
+#include <string>
+#include <vector>
using ::testing::AllOf;
using ::testing::HasSubstr;
@@ -554,7 +557,6 @@
EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail"));
// Don't extract return
EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail"));
-
}
TEST_F(ExtractFunctionTest, FileTest) {
@@ -648,6 +650,150 @@
EXPECT_THAT(apply(" for(;;) { [[while(1) break; break;]] }"),
StartsWith("fail"));
}
+
+TWEAK_TEST(DefineInline);
+TEST_F(DefineInlineTest, TriggersOnFunctionDecl) {
+ // Basic check for function body and signature.
+ EXPECT_AVAILABLE(R"cpp(
+ class Bar {
+ void baz();
+ };
+
+ [[void [[Bar::[[b^a^z]]]]() [[{
+ return;
+ }]]]]
+
+ void foo();
+ [[void [[f^o^o]]() [[{
+ return;
+ }]]]]
+ )cpp");
+
+ EXPECT_UNAVAILABLE(R"cpp(
+ // Not a definition
+ vo^i[[d^ ^f]]^oo();
+
+ [[vo^id ]]foo[[()]] {[[
+ [[(void)(5+3);
+ return;]]
+ }]]
+ )cpp");
+}
+
+TEST_F(DefineInlineTest, NoForwardDecl) {
+ Header = "void bar();";
+ EXPECT_UNAVAILABLE(R"cpp(
+ void bar() {
+ return;
+ }
+ // FIXME: Generate a decl in the header.
+ void fo^o() {
+ return;
+ })cpp");
+}
+
+TEST_F(DefineInlineTest, ReferencedDecls) {
+ EXPECT_AVAILABLE(R"cpp(
+ void bar();
+ void foo(int test);
+
+ void fo^o(int baz) {
+ int x = 10;
+ bar();
+ })cpp");
+
+ // Internal symbol usage.
+ Header = "void foo(int test);";
+ EXPECT_UNAVAILABLE(R"cpp(
+ void bar();
+ void fo^o(int baz) {
+ int x = 10;
+ bar();
+ })cpp");
+
+ // Becomes available after making symbol visible.
+ Header = "void bar();" + Header;
+ EXPECT_AVAILABLE(R"cpp(
+ void fo^o(int baz) {
+ int x = 10;
+ bar();
+ })cpp");
+
+ // FIXME: Move declaration below bar to make it visible.
+ Header.clear();
+ EXPECT_UNAVAILABLE(R"cpp(
+ void foo();
+ void bar();
+
+ void fo^o() {
+ bar();
+ })cpp");
+
+ // Order doesn't matter within a class.
+ EXPECT_AVAILABLE(R"cpp(
+ class Bar {
+ void foo();
+ void bar();
+ };
+
+ void Bar::fo^o() {
+ bar();
+ })cpp");
+
+ // FIXME: Perform include insertion to make symbol visible.
+ ExtraFiles["a.h"] = "void bar();";
+ Header = "void foo(int test);";
+ EXPECT_UNAVAILABLE(R"cpp(
+ #include "a.h"
+ void fo^o(int baz) {
+ int x = 10;
+ bar();
+ })cpp");
+}
+
+TEST_F(DefineInlineTest, TemplateSpec) {
+ EXPECT_UNAVAILABLE(R"cpp(
+ template <typename T> void foo();
+ template<> void foo<char>();
+
+ template<> void f^oo<int>() {
+ })cpp");
+ EXPECT_UNAVAILABLE(R"cpp(
+ template <typename T> void foo();
+
+ template<> void f^oo<int>() {
+ })cpp");
+ EXPECT_AVAILABLE(R"cpp(
+ template <typename T> void foo();
+ void bar();
+ template <> void foo<int>();
+
+ template<> void f^oo<int>() {
+ bar();
+ })cpp");
+}
+
+TEST_F(DefineInlineTest, CheckForCanonDecl) {
+ EXPECT_UNAVAILABLE(R"cpp(
+ void foo();
+
+ void bar() {}
+ void f^oo() {
+ // This bar normally refers to the definition just above, but it is not
+ // visible from the forward declaration of foo.
+ bar();
+ })cpp");
+ // Make it available with a forward decl.
+ EXPECT_AVAILABLE(R"cpp(
+ void bar();
+ void foo();
+
+ void bar() {}
+ void f^oo() {
+ bar();
+ })cpp");
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/TweakTesting.h
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/TweakTesting.h
@@ -10,8 +10,10 @@
#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TWEAKTESTING_H
#include "TestTU.h"
-#include "gtest/gtest.h"
+#include "llvm/ADT/StringMap.h"
#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <string>
namespace clang {
namespace clangd {
@@ -47,6 +49,9 @@
Expression,
};
+ // Mapping from file name to contents.
+ llvm::StringMap<std::string> ExtraFiles;
+
protected:
TweakTest(const char *TweakID) : TweakID(TweakID) {}
Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/TweakTesting.cpp
@@ -59,7 +59,7 @@
cantFail(positionToOffset(A.code(), SelectionRng.end))};
}
-MATCHER_P3(TweakIsAvailable, TweakID, Ctx, Header,
+MATCHER_P4(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles,
(TweakID + (negation ? " is unavailable" : " is available")).str()) {
std::string WrappedCode = wrap(Ctx, arg);
Annotations Input(WrappedCode);
@@ -67,6 +67,7 @@
TestTU TU;
TU.HeaderCode = Header;
TU.Code = Input.code();
+ TU.AdditionalFiles = std::move(ExtraFiles);
ParsedAST AST = TU.build();
Tweak::Selection S(AST, Selection.first, Selection.second);
auto PrepareResult = prepareTweak(TweakID, S);
@@ -111,7 +112,8 @@
}
::testing::Matcher<llvm::StringRef> TweakTest::isAvailable() const {
- return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header);
+ return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header,
+ ExtraFiles);
}
std::vector<std::string> TweakTest::expandCases(llvm::StringRef MarkedCode) {
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -0,0 +1,207 @@
+//===--- DefineInline.cpp ----------------------------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AST.h"
+#include "Logger.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "XRefs.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/PrettyPrinter.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/TemplateBase.h"
+#include "clang/AST/Type.h"
+#include "clang/AST/TypeLoc.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TokenKinds.h"
+#include "clang/Index/IndexDataConsumer.h"
+#include "clang/Index/IndexSymbol.h"
+#include "clang/Index/IndexingAction.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/Token.h"
+#include "clang/Sema/Lookup.h"
+#include "clang/Sema/Sema.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/raw_ostream.h"
+#include <cstddef>
+#include <set>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+namespace clang {
+namespace clangd {
+namespace {
+
+// Deduces the FunctionDecl from a selection. Requires either the function body
+// or the function decl to be selected. Returns null if none of the above
+// criteria is met.
+const FunctionDecl *getSelectedFunction(const SelectionTree::Node *SelNode) {
+ const ast_type_traits::DynTypedNode &AstNode = SelNode->ASTNode;
+ if (const FunctionDecl *FD = AstNode.get<FunctionDecl>())
+ return FD;
+ if (AstNode.get<CompoundStmt>() &&
+ SelNode->Selected == SelectionTree::Complete) {
+ if (const SelectionTree::Node *P = SelNode->Parent)
+ return P->ASTNode.get<FunctionDecl>();
+ }
+ return nullptr;
+}
+
+// Checks the decls mentioned in Source are visible in the context of Target.
+// Achives that by checking declarations occur before target location in
+// translation unit or declared in the same class.
+bool checkDeclsAreVisible(const llvm::DenseSet<const Decl *> &DeclRefs,
+ const FunctionDecl *Target, const SourceManager &SM) {
+ SourceLocation TargetLoc = Target->getLocation();
+ // To be used in visibility check below, decls in a class are visible
+ // independent of order.
+ const RecordDecl *Class = nullptr;
+ if (const auto *MD = llvm::dyn_cast<CXXMethodDecl>(Target))
+ Class = MD->getParent();
+
+ for (const auto *DR : DeclRefs) {
+ // Use canonical decl, since having one decl before target is enough.
+ const Decl *D = DR->getCanonicalDecl();
+ if (D == Target)
+ continue;
+ SourceLocation DeclLoc = D->getLocation();
+
+ // FIXME: Allow declarations from different files with include insertion.
+ if (!SM.isWrittenInSameFile(DeclLoc, TargetLoc))
+ return false;
+
+ // If declaration is before target, then it is visible.
+ if (SM.isBeforeInTranslationUnit(DeclLoc, TargetLoc))
+ continue;
+
+ // Otherwise they need to be in same class
+ if (!Class)
+ return false;
+ const RecordDecl *Parent = nullptr;
+ if (const auto *MD = llvm::dyn_cast<CXXMethodDecl>(D))
+ Parent = MD->getParent();
+ else if (const auto *FD = llvm::dyn_cast<FieldDecl>(D))
+ Parent = FD->getParent();
+ if (Parent != Class)
+ return false;
+ }
+ return true;
+}
+
+// Returns the canonical declaration for the given FunctionDecl. This will
+// usually be the first declaration in current translation unit with the
+// exception of template specialization.
+// For those we return first declaration different than the canonical one.
+// Because canonical declaration points to template decl instead of
+// specialization.
+const FunctionDecl *findTarget(const FunctionDecl *FD) {
+ auto CanonDecl = FD->getCanonicalDecl();
+ if (!FD->isFunctionTemplateSpecialization())
+ return CanonDecl;
+ // For specializations CanonicalDecl is the TemplatedDecl, which is not the
+ // target we want to inline into. Instead we traverse previous decls to find
+ // the first forward decl for this specialization.
+ auto PrevDecl = FD;
+ while (PrevDecl->getPreviousDecl() != CanonDecl) {
+ PrevDecl = PrevDecl->getPreviousDecl();
+ assert(PrevDecl && "Found specialization without template decl");
+ }
+ return PrevDecl;
+}
+
+/// Moves definition of a function/method to its declaration location.
+/// Before:
+/// a.h:
+/// void foo();
+///
+/// a.cc:
+/// void foo() { return; }
+///
+/// ------------------------
+/// After:
+/// a.h:
+/// void foo() { return; }
+///
+/// a.cc:
+///
+class DefineInline : public Tweak {
+public:
+ const char *id() const override final;
+
+ Intent intent() const override { return Intent::Refactor; }
+ std::string title() const override {
+ return "Move function body to declaration";
+ }
+ bool hidden() const override { return true; }
+
+ // Returns true when selection is on a function definition that does not
+ // make use of any internal symbols.
+ bool prepare(const Selection &Sel) override {
+ const SelectionTree::Node *SelNode = Sel.ASTSelection.commonAncestor();
+ if (!SelNode)
+ return false;
+ Source = getSelectedFunction(SelNode);
+ if (!Source || !Source->isThisDeclarationADefinition())
+ return false;
+
+ Target = findTarget(Source);
+ if (Target == Source) {
+ // The only declaration is Source. No other declaration to move function
+ // body.
+ // FIXME: If we are in an implementation file, figure out a suitable
+ // location to put declaration. Possibly using other declarations in the
+ // AST.
+ return false;
+ }
+
+ // Check if the decls referenced in function body are visible in the
+ // declaration location.
+ if (!checkDeclsAreVisible(getNonLocalDeclRefs(Sel.AST, Source), Target,
+ Sel.AST.getSourceManager()))
+ return false;
+
+ return true;
+ }
+
+ Expected<Effect> apply(const Selection &Sel) override {
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Not implemented yet");
+ }
+
+private:
+ const FunctionDecl *Source = nullptr;
+ const FunctionDecl *Target = nullptr;
+};
+
+REGISTER_TWEAK(DefineInline);
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -14,6 +14,7 @@
add_clang_library(clangDaemonTweaks OBJECT
AnnotateHighlightings.cpp
DumpAST.cpp
+ DefineInline.cpp
ExpandAutoType.cpp
ExpandMacro.cpp
ExtractFunction.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
