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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
