kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:262
+ ID != SM.getMainFileID() && FE &&
+ !isSelfContainedHeader(PP, ID, FE);) {
+ ID = SM.getFileID(SM.getIncludeLoc(ID));
----------------
kbobyrev wrote:
> sammccall wrote:
> > 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.
> I am slightly confused: we don't really have the `IncludeStructure` here and
> it is logically detached from the `IncludeStructure` processing. We'd still
> have to unroll the chain of FIDs in here, so the only difference would be
> querying `IncludeStructure` data for the cached `isSelfContainedHeader`
> value, is that right? Why is obtaining that info at preamble build time
> better?
the call-site has access to ParsedAST, hence the IncludeStructure. I believe
the main reasoning behind Sam's suggestion is performing these lookups once
while building the preamble and sharing it afterwards (we can even do the IO
there).
================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:56
+// Is Line an #if or #ifdef directive?
+static bool isIf(llvm::StringRef Line) {
+ Line = Line.ltrim();
----------------
no need for static here (and other places below)
================
Comment at: clang-tools-extra/clangd/SourceCode.h:332
+bool isSelfContainedHeader(const Preprocessor &PP, FileID FID,
+ const FileEntry *FE, bool ExpensiveCheck = false);
+
----------------
i'd suggest `AllowIO` rather than `ExpensiveCheck`
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:283
// Is Line an #if or #ifdef directive?
static bool isIf(llvm::StringRef Line) {
Line = Line.ltrim();
----------------
can you also delete this and the other helpers?
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:346
+ // headers to header-guarded ones.
+ for (const auto &Key : ReferencedFileNames.keys()) {
+ llvm::outs() << "Key: " << Key << '\n';
----------------
looks like a debug artifact
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
