sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:252
}
- return std::move(Result.Files);
+ // Post-filtering attributes the locations from non self-contained headers to
+ // their parents while the information about respective SourceLocations and
----------------
This comment seems a bit unclear:
- what the problem is you're solving
- whether you're describing the behavior you *want* rather than the behavior
you're trying to avoid.
- which part of this is "filtering"
- what the "later" option is you're contrasting to
Maybe something like:
```
// If a header is not self-contained, we consider its symbols a logical part of
the including file.
// Therefore, mark the parents of all used non-self-contained FileIDs as used.
// Perform this on FileIDs rather than HeaderIDs, as each inclusion of a
non-self-contained file is distinct.
```
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:262
+ ID != SM.getMainFileID() && FE &&
+ !isSelfContainedHeader(PP, ID, FE);) {
+ ID = SM.getFileID(SM.getIncludeLoc(ID));
----------------
it seems like we'd be better off storing the "is-self-contained" in the
IncludeStructure and looking up the HeaderID here rather than asking the
preprocessor. That way we rely on info that's better obtained at preamble build
time.
================
Comment at: clang-tools-extra/clangd/SourceCode.h:330
+/// preprocessor state).
+bool isSelfContainedHeader(const Preprocessor &PP, FileID FID,
+ const FileEntry *FE);
----------------
This helper checks e.g. for "don't include me", which is going to read source
code of preamble files - we shouldn't do that, it's too slow and racy to do for
every file in the preamble.
(It would be nice to handle those cases at some point, if we want to do that we
need to do it at preamble build time and record the results in the
IncludeStructure)
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
