[PATCH] D114370: [clangd] IncludeCleaner: Attribute symbols from non self-contained headers to their parents

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, LG in general, just a couple polishing touches



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:146
+  //
+  // Returns true if the insertion into IDs took place.
+  bool insert(llvm::DenseSet<FileID> &IDs, FileID ID, const SourceManager &SM,
----------------
this returns true even after inserting mainfile id, is that intended? I suppose it won't hurt too much, but means we'll go one step further for macros.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:155
+         ID != SM.getMainFileID() && FE &&
+         !PP.getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE);) {
+      ID = SM.getFileID(SM.getIncludeLoc(ID));
----------------
can you lift the [isSelfContainedHeader](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/index/SymbolCollector.cpp#L282) into SourceCode.h and make use of it in both places (while updating the signature to accept a PP, through which you can access the SM).


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:162
+
+  void add(SourceLocation Loc, const SourceManager &SM,
+           const Preprocessor &PP) {
----------------
we already have access to `SM` from the members, can you do the same for `PP` (stash as a member during construction) rather than plumbing at each call?


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:350
+
+  auto ReferencedHeaders = translateToHeaderIDs(ReferencedFiles, Includes, SM);
+  std::vector<llvm::StringRef> ReferencedHeaderNames;
----------------
is this part of the check even relevant? let's drop this completely?


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:355
+  EXPECT_THAT(ReferencedHeaderNames,
+              ElementsAre(testPath("bar.h"), testPath("foo.h")));
+
----------------
maybe make this an `UnorderedElementsAre`, do we have any ordering guarantees in this API's contract?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370