[PATCH] D128457: [clangd] Add new IncludeType to IncludeHeaderWithReferences

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:1241
+      !HeaderInfo.hasFileBeenImported(FE) &&
+      (FID != SM.getMainFileID() || !fileContainsImports(FID, SM)))
     return false;
----------------
can you put a comment here saying `any header that contains #imports are supposed to be #import'd so no need to check for anything but the main-file.`


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:1249
+bool fileContainsImports(FileID ID, const SourceManager &SM) {
+  llvm::StringRef Content = SM.getBufferData(ID);
+  llvm::StringRef Line;
----------------
nit: perform `.take_front` directly here.


================
Comment at: clang-tools-extra/clangd/index/Merge.cpp:251
         SI.References += OI.References;
+        SI.SupportedDirectives =
+            SI.SupportedDirectives | OI.SupportedDirectives;
----------------
nit: `SI.SupportedDirectives |= OI.SupportedDirectives;`


================
Comment at: clang-tools-extra/clangd/index/Symbol.h:90
 
+  enum IncludeDirectives : uint8_t {
+    Invalid = 0,
----------------
nit: I'd still keep the enum name singular


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:103
+  using SK = index::SymbolKind;
+  switch (Kind) {
+  case SK::Macro:
----------------
can we rather modify the previous function to look like:
```
IncludeDirective shouldCollectIncludePath(index::SymbolKind Kind) {
  using SK = index::SymbolKind;
  switch (Kind) {
  case SK::Macro:
  case SK::Enum:
  case SK::Struct:
  case SK::Class:
  case SK::Union:
  case SK::TypeAlias:
  case SK::Using:
  case SK::Function:
  case SK::Variable:
  case SK::EnumConstant:
  case SK::Concept:
    return Include | Import;
  case SK::Protocol:
    return Import;
  default:
    return Invalid;
  }
}
```


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:838
+      IncludeFiles[S.ID] = FID;
+      if (S.SymInfo.Lang == index::SymbolLanguage::ObjC)
+        FilesWithObjCConstructs.insert(FID);
----------------
can we do this in `addDeclaration` instead? we already have nameLocation and FileID there, but that's also where we have the decl itself, which might be needed in future if we need more detailed checks.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:886
       }
+      bool ContainsImports;
+      auto It = FileToContainsImports.find(Entry.second);
----------------
can we instead do
```
auto It = map.find(key);
if (It == map.end()) {
  It = map.insert(key, calculateValue()).first;
}
bool ContainsImports = It->second;
```


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:905
+        if (shouldCollectImportPath(S->SymInfo.Kind) &&
+            (ContainsImports || FilesWithObjCConstructs.contains(Entry.second)))
+          Directives |= Symbol::Import;
----------------
can we perform `ContainsImports` calculations/caching here instead? that way we can trim a bunch of redundant searches.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:431
   }
-  return clangd::Symbol::IncludeHeaderWithReferences{Strings.save(Header),
-                                                     Message.references()};
+  auto IncludeTypes = fromProtobuf(Message.include_types());
+  if (!IncludeTypes)
----------------
i guess `IncludeTypes` pieces are leftover?


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2679
+  assert(!Results.empty());
+  // TODO: Once #import integration support is done this shouldn't include
+  // anything.
----------------
i think this case should still be empty, otherwise we're actually regressing the behaviour by inserting includes for symbols that we previously wouldn't insert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128457