[PATCH] D70299: [clangd] Replace getLangOpts().isHeaderFile usage with isHeaderFile helper.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
Closed by commit rGb221c9d09dd1: [clangd] Replace getLangOpts().isHeaderFile 
usage with isHeaderFile helper. (authored by hokein).

Changed prior to commit:
  https://reviews.llvm.org/D70299?vs=229487=229551#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70299/new/

https://reviews.llvm.org/D70299

Files:
  clang-tools-extra/clangd/HeaderSourceSwitch.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -664,6 +664,17 @@
   runSymbolCollector("", Header.code(),
  /*ExtraArgs=*/{"-xobjective-c++-header"});
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"), QName("Func")));
+  EXPECT_THAT(Refs,
+  UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
+HaveRanges(Header.ranges("Foo"))),
+   Pair(findSymbol(Symbols, "Func").ID,
+HaveRanges(Header.ranges("Func");
+
+  // Run the .hh file as main file (without "-x c++-header"), we should collect
+  // the refs as well.
+  TestFileName = testPath("foo.hh");
+  runSymbolCollector("", Header.code());
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"), QName("Func")));
   EXPECT_THAT(Refs, UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
   HaveRanges(Header.ranges("Foo"))),
  Pair(findSymbol(Symbols, "Func").ID,
Index: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
+++ clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
@@ -178,14 +178,11 @@
 }
 
 TEST_F(WorkspaceSymbolsTest, AnonymousNamespace) {
-  addFile("foo.h", R"cpp(
+  addFile("foo.cpp", R"cpp(
   namespace {
   void test() {}
   }
   )cpp");
-  addFile("foo.cpp", R"cpp(
-  #include "foo.h"
-  )cpp");
   EXPECT_THAT(getSymbols("test"), ElementsAre(QName("test")));
 }
 
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -76,7 +76,7 @@
   }
   auto  = RenameDecl.getASTContext();
   const auto  = ASTCtx.getSourceManager();
-  bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile;
+  bool MainFileIsHeader = isHeaderFile(MainFile, ASTCtx.getLangOpts());
   bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM);
 
   if (!DeclaredInMainFile)
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -304,7 +304,8 @@
   // it's main-file only.
   bool IsMainFileOnly =
   SM.isWrittenInMainFile(SM.getExpansionLoc(ND->getBeginLoc())) &&
-  !ASTCtx->getLangOpts().IsHeaderFile;
+  !isHeaderFile(SM.getFileEntryForID(SM.getMainFileID())->getName(),
+ASTCtx->getLangOpts());
   // In C, printf is a redecl of an implicit builtin! So check OrigD instead.
   if (ASTNode.OrigD->isImplicit() ||
   !shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
Index: clang-tools-extra/clangd/HeaderSourceSwitch.cpp
===
--- clang-tools-extra/clangd/HeaderSourceSwitch.cpp
+++ clang-tools-extra/clangd/HeaderSourceSwitch.cpp
@@ -9,6 +9,7 @@
 #include "HeaderSourceSwitch.h"
 #include "AST.h"
 #include "Logger.h"
+#include "SourceCode.h"
 #include "index/SymbolCollector.h"
 #include "clang/AST/Decl.h"
 
@@ -96,7 +97,7 @@
   //
   // For each symbol in the original file, we get its target location (decl or
   // def) from the index, then award that target file.
-  bool IsHeader = AST.getASTContext().getLangOpts().IsHeaderFile;
+  bool IsHeader = isHeaderFile(OriginalFile, 
AST.getASTContext().getLangOpts());
   Index->lookup(Request, [&](const Symbol ) {
 if (IsHeader)
   AwardTarget(Sym.Definition.FileURI);


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -664,6 

[PATCH] D70299: [clangd] Replace getLangOpts().isHeaderFile usage with isHeaderFile helper.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:181
 TEST_F(WorkspaceSymbolsTest, AnonymousNamespace) {
-  addFile("foo.h", R"cpp(
+  // header symbols in anonymous namespace are not collected.
+  addFile("foo.cpp", R"cpp(

I'm not sure what the purpose of this comment is (in reference to the code, 
rather than to this change) - just remove it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70299/new/

https://reviews.llvm.org/D70299



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70299: [clangd] Replace getLangOpts().isHeaderFile usage with isHeaderFile helper.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 60093 tests passed, 0 failed and 729 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70299/new/

https://reviews.llvm.org/D70299



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70299: [clangd] Replace getLangOpts().isHeaderFile usage with isHeaderFile helper.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.
hokein added a parent revision: D70235: [clangd] Add isHeaderFile helper..

The helper is more correct to detect header file, this would fix our
issues caused by false positive before.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70299

Files:
  clang-tools-extra/clangd/HeaderSourceSwitch.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -664,6 +664,17 @@
   runSymbolCollector("", Header.code(),
  /*ExtraArgs=*/{"-xobjective-c++-header"});
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"), QName("Func")));
+  EXPECT_THAT(Refs,
+  UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
+HaveRanges(Header.ranges("Foo"))),
+   Pair(findSymbol(Symbols, "Func").ID,
+HaveRanges(Header.ranges("Func");
+
+  // Run the .hh file as main file (without "-x c++-header"), we should collect
+  // the refs as well.
+  TestFileName = testPath("foo.hh");
+  runSymbolCollector("", Header.code());
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"), QName("Func")));
   EXPECT_THAT(Refs, UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
   HaveRanges(Header.ranges("Foo"))),
  Pair(findSymbol(Symbols, "Func").ID,
Index: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
+++ clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
@@ -178,14 +178,12 @@
 }
 
 TEST_F(WorkspaceSymbolsTest, AnonymousNamespace) {
-  addFile("foo.h", R"cpp(
+  // header symbols in anonymous namespace are not collected.
+  addFile("foo.cpp", R"cpp(
   namespace {
   void test() {}
   }
   )cpp");
-  addFile("foo.cpp", R"cpp(
-  #include "foo.h"
-  )cpp");
   EXPECT_THAT(getSymbols("test"), ElementsAre(QName("test")));
 }
 
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -76,7 +76,7 @@
   }
   auto  = RenameDecl.getASTContext();
   const auto  = ASTCtx.getSourceManager();
-  bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile;
+  bool MainFileIsHeader = isHeaderFile(MainFile, ASTCtx.getLangOpts());
   bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM);
 
   if (!DeclaredInMainFile)
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -304,7 +304,8 @@
   // it's main-file only.
   bool IsMainFileOnly =
   SM.isWrittenInMainFile(SM.getExpansionLoc(ND->getBeginLoc())) &&
-  !ASTCtx->getLangOpts().IsHeaderFile;
+  !isHeaderFile(SM.getFileEntryForID(SM.getMainFileID())->getName(),
+ASTCtx->getLangOpts());
   // In C, printf is a redecl of an implicit builtin! So check OrigD instead.
   if (ASTNode.OrigD->isImplicit() ||
   !shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
Index: clang-tools-extra/clangd/HeaderSourceSwitch.cpp
===
--- clang-tools-extra/clangd/HeaderSourceSwitch.cpp
+++ clang-tools-extra/clangd/HeaderSourceSwitch.cpp
@@ -9,6 +9,7 @@
 #include "HeaderSourceSwitch.h"
 #include "AST.h"
 #include "Logger.h"
+#include "SourceCode.h"
 #include "index/SymbolCollector.h"
 #include "clang/AST/Decl.h"
 
@@ -96,7 +97,7 @@
   //
   // For each symbol in the original file, we get its target location (decl or
   // def) from the index, then award that target file.
-  bool IsHeader = AST.getASTContext().getLangOpts().IsHeaderFile;
+  bool IsHeader = isHeaderFile(OriginalFile, 
AST.getASTContext().getLangOpts());
   Index->lookup(Request, [&](const Symbol ) {
 if (IsHeader)
   AwardTarget(Sym.Definition.FileURI);


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++