From 6ae86ea2752126d386206cd92ef6a905f6f58598 Mon Sep 17 00:00:00 2001 From: Haojian Wu Date: Fri, 19 Jul 2019 08:33:39 +0000 Subject: [PATCH] [clangd] cleanup: unify the implemenation of checking a location is inside main file. Summary: We have variant implementations in the codebase, this patch unifies them. Reviewers: ilya-biryukov, kadircet Subscribers: MaskRay, jkorous, arphaman, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D64915 llvm-svn: 366541 --- clang-tools-extra/clangd/ClangdUnit.cpp | 5 ++-- clang-tools-extra/clangd/Diagnostics.cpp | 6 +--- clang-tools-extra/clangd/Headers.cpp | 2 +- clang-tools-extra/clangd/IncludeFixer.cpp | 2 +- clang-tools-extra/clangd/Quality.cpp | 7 ++--- clang-tools-extra/clangd/SourceCode.cpp | 4 +++ clang-tools-extra/clangd/SourceCode.h | 8 +++++ clang-tools-extra/clangd/XRefs.cpp | 2 +- .../clangd/index/SymbolCollector.cpp | 3 +- clang-tools-extra/clangd/refactor/Rename.cpp | 3 +- .../clangd/unittests/SourceCodeTests.cpp | 30 +++++++++++++++++++ .../clangd/unittests/SymbolCollectorTests.cpp | 3 +- 12 files changed, 54 insertions(+), 21 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdUnit.cpp b/clang-tools-extra/clangd/ClangdUnit.cpp index b337d851d0f7f..3e9db706cfade 100644 --- a/clang-tools-extra/clangd/ClangdUnit.cpp +++ b/clang-tools-extra/clangd/ClangdUnit.cpp @@ -68,7 +68,7 @@ class DeclTrackingASTConsumer : public ASTConsumer { bool HandleTopLevelDecl(DeclGroupRef DG) override { for (Decl *D : DG) { auto &SM = D->getASTContext().getSourceManager(); - if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation()))) + if (!isInsideMainFile(D->getLocation(), SM)) continue; // ObjCMethodDecl are not actually top-level decls. @@ -355,8 +355,7 @@ ParsedAST::build(std::unique_ptr CI, // those might take us into a preamble file as well. bool IsInsideMainFile = Info.hasSourceManager() && - Info.getSourceManager().isWrittenInMainFile( - Info.getSourceManager().getFileLoc(Info.getLocation())); + isInsideMainFile(Info.getLocation(), Info.getSourceManager()); if (IsInsideMainFile && tidy::ShouldSuppressDiagnostic( DiagLevel, Info, *CTContext, /* CheckMacroExpansion = */ false)) { diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index 88bd6fdcad115..53192711a5df7 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -140,15 +140,11 @@ void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info, D.Message = llvm::Twine("in included file: ", D.Message).str(); } -bool isInsideMainFile(const SourceLocation Loc, const SourceManager &M) { - return Loc.isValid() && M.isWrittenInMainFile(M.getFileLoc(Loc)); -} - bool isInsideMainFile(const clang::Diagnostic &D) { if (!D.hasSourceManager()) return false; - return isInsideMainFile(D.getLocation(), D.getSourceManager()); + return clangd::isInsideMainFile(D.getLocation(), D.getSourceManager()); } bool isNote(DiagnosticsEngine::Level L) { diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp index 877301ee2d921..7caaebaa6f30e 100644 --- a/clang-tools-extra/clangd/Headers.cpp +++ b/clang-tools-extra/clangd/Headers.cpp @@ -35,7 +35,7 @@ class RecordHeaders : public PPCallbacks { llvm::StringRef /*RelativePath*/, const Module * /*Imported*/, SrcMgr::CharacteristicKind FileKind) override { - if (SM.isWrittenInMainFile(HashLoc)) { + if (isInsideMainFile(HashLoc, SM)) { Out->MainFileIncludes.emplace_back(); auto &Inc = Out->MainFileIncludes.back(); Inc.R = halfOpenToRange(SM, FilenameRange); diff --git a/clang-tools-extra/clangd/IncludeFixer.cpp b/clang-tools-extra/clangd/IncludeFixer.cpp index edf86504d139d..99c55c6cd6242 100644 --- a/clang-tools-extra/clangd/IncludeFixer.cpp +++ b/clang-tools-extra/clangd/IncludeFixer.cpp @@ -318,7 +318,7 @@ class IncludeFixer::UnresolvedNameRecorder : public ExternalSemaSource { assert(SemaPtr && "Sema must have been set."); if (SemaPtr->isSFINAEContext()) return TypoCorrection(); - if (!SemaPtr->SourceMgr.isWrittenInMainFile(Typo.getLoc())) + if (!isInsideMainFile(Typo.getLoc(), SemaPtr->SourceMgr)) return clang::TypoCorrection(); // This is not done lazily because `SS` can get out of scope and it's diff --git a/clang-tools-extra/clangd/Quality.cpp b/clang-tools-extra/clangd/Quality.cpp index 6ab05293274f9..bd25256904cde 100644 --- a/clang-tools-extra/clangd/Quality.cpp +++ b/clang-tools-extra/clangd/Quality.cpp @@ -9,6 +9,7 @@ #include "Quality.h" #include "AST.h" #include "FileDistance.h" +#include "SourceCode.h" #include "URI.h" #include "index/Symbol.h" #include "clang/AST/ASTContext.h" @@ -42,8 +43,7 @@ static bool isReserved(llvm::StringRef Name) { static bool hasDeclInMainFile(const Decl &D) { auto &SourceMgr = D.getASTContext().getSourceManager(); for (auto *Redecl : D.redecls()) { - auto Loc = SourceMgr.getSpellingLoc(Redecl->getLocation()); - if (SourceMgr.isWrittenInMainFile(Loc)) + if (isInsideMainFile(Redecl->getLocation(), SourceMgr)) return true; } return false; @@ -53,8 +53,7 @@ static bool hasUsingDeclInMainFile(const CodeCompletionResult &R) { const auto &Context = R.Declaration->getASTContext(); const auto &SourceMgr = Context.getSourceManager(); if (R.ShadowDecl) { - const auto Loc = SourceMgr.getExpansionLoc(R.ShadowDecl->getLocation()); - if (SourceMgr.isWrittenInMainFile(Loc)) + if (isInsideMainFile(R.ShadowDecl->getLocation(), SourceMgr)) return true; } return false; diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp index fb1183aef5115..d9911647a5fc2 100644 --- a/clang-tools-extra/clangd/SourceCode.cpp +++ b/clang-tools-extra/clangd/SourceCode.cpp @@ -328,6 +328,10 @@ static SourceRange getTokenFileRange(SourceLocation Loc, return FileRange; } +bool isInsideMainFile(SourceLocation Loc, const SourceManager &SM) { + return Loc.isValid() && SM.isWrittenInMainFile(SM.getExpansionLoc(Loc)); +} + llvm::Optional toHalfOpenFileRange(const SourceManager &SM, const LangOptions &LangOpts, SourceRange R) { diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h index 8e58e66f4db8d..4bbf3cf49fcd3 100644 --- a/clang-tools-extra/clangd/SourceCode.h +++ b/clang-tools-extra/clangd/SourceCode.h @@ -75,6 +75,14 @@ llvm::Optional getTokenRange(const SourceManager &SM, llvm::Expected sourceLocationInMainFile(const SourceManager &SM, Position P); +/// Returns true iff \p Loc is inside the main file. This function handles +/// file & macro locations. For macro locations, returns iff the macro is being +/// expanded inside the main file. +/// +/// The function is usually used to check whether a declaration is inside the +/// the main file. +bool isInsideMainFile(SourceLocation Loc, const SourceManager &SM); + /// Turns a token range into a half-open range and checks its correctness. /// The resulting range will have only valid source location on both sides, both /// of which are file locations. diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp index 6339f8643f745..ba9e662f93456 100644 --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -406,7 +406,7 @@ class ReferenceFinder : public index::IndexDataConsumer { assert(D->isCanonicalDecl() && "expect D to be a canonical declaration"); const SourceManager &SM = AST.getSourceManager(); Loc = SM.getFileLoc(Loc); - if (SM.isWrittenInMainFile(Loc) && CanonicalTargets.count(D)) + if (isInsideMainFile(Loc, SM) && CanonicalTargets.count(D)) References.push_back({D, Loc, Roles}); return true; } diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp index 8b2d40b3d3890..cca8b004ca362 100644 --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -185,8 +185,7 @@ getTokenLocation(SourceLocation TokLoc, const SourceManager &SM, bool isPreferredDeclaration(const NamedDecl &ND, index::SymbolRoleSet Roles) { const auto &SM = ND.getASTContext().getSourceManager(); return (Roles & static_cast(index::SymbolRole::Definition)) && - isa(&ND) && - !SM.isWrittenInMainFile(SM.getExpansionLoc(ND.getLocation())); + isa(&ND) && !isInsideMainFile(ND.getLocation(), SM); } RefKind toRefKind(index::SymbolRoleSet Roles) { diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp index d54fae5191407..b66eac4ee8da7 100644 --- a/clang-tools-extra/clangd/refactor/Rename.cpp +++ b/clang-tools-extra/clangd/refactor/Rename.cpp @@ -104,8 +104,7 @@ llvm::Optional renamableWithinFile(const Decl &RenameDecl, auto &ASTCtx = RenameDecl.getASTContext(); const auto &SM = ASTCtx.getSourceManager(); bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile; - bool DeclaredInMainFile = - SM.isWrittenInMainFile(SM.getExpansionLoc(RenameDecl.getLocation())); + bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM); // If the symbol is declared in the main file (which is not a header), we // rename it. diff --git a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp index 29eda85bc458d..6f937e6e6b269 100644 --- a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp +++ b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp @@ -422,6 +422,36 @@ TEST(SourceCodeTests, GetMacros) { EXPECT_THAT(*Result, MacroName("MACRO")); } +TEST(SourceCodeTests, IsInsideMainFile){ + TestTU TU; + TU.HeaderCode = R"cpp( + #define DEFINE_CLASS(X) class X {}; + #define DEFINE_YY DEFINE_CLASS(YY) + + class Header1 {}; + DEFINE_CLASS(Header2) + class Header {}; + )cpp"; + TU.Code = R"cpp( + class Main1 {}; + DEFINE_CLASS(Main2) + DEFINE_YY + class Main {}; + )cpp"; + TU.ExtraArgs.push_back("-DHeader=Header3"); + TU.ExtraArgs.push_back("-DMain=Main3"); + auto AST = TU.build(); + const auto& SM = AST.getSourceManager(); + auto DeclLoc = [&AST](llvm::StringRef Name) { + return findDecl(AST, Name).getLocation(); + }; + for (const auto *HeaderDecl : {"Header1", "Header2", "Header3"}) + EXPECT_FALSE(isInsideMainFile(DeclLoc(HeaderDecl), SM)); + + for (const auto *MainDecl : {"Main1", "Main2", "Main3", "YY"}) + EXPECT_TRUE(isInsideMainFile(DeclLoc(MainDecl), SM)); +} + // Test for functions toHalfOpenFileRange and getHalfOpenFileRange TEST(SourceCodeTests, HalfOpenFileRange) { // Each marked range should be the file range of the decl with the same name diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp index 8c88fe5169d8a..8caa1c5aa7323 100644 --- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp +++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp @@ -124,8 +124,7 @@ class ShouldCollectSymbolTest : public ::testing::Test { const NamedDecl &ND = Qualified ? findDecl(*AST, Name) : findUnqualifiedDecl(*AST, Name); const SourceManager &SM = AST->getSourceManager(); - bool MainFile = - SM.isWrittenInMainFile(SM.getExpansionLoc(ND.getBeginLoc())); + bool MainFile = isInsideMainFile(ND.getBeginLoc(), SM); return SymbolCollector::shouldCollectSymbol( ND, AST->getASTContext(), SymbolCollector::Options(), MainFile); }