jansvoboda11 added a comment. I finished an initial pass through the code and it looks good for the most parts. I left a couple of suggestions in-line.
Did you test this change on some larger body of code? That would make me more confident, since this is a non-trivial change. Might be good to get other reviewers to chime in. ================ Comment at: clang/include/clang/Lex/PreprocessorOptions.h:214 + FileEntryRef)> + DependencyDirectivesForFile; ---------------- To be honest, I'm not a fan of using `PreprocessorOptions` to carry state between compiler invocations. Could we implement a different mechanism for this in a prep patch an put `DependencyDirectivesForFile` there? `FailedModules` also seem as a state rather than options. ================ Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:1 -//===- DependencyDirectivesSourceMinimizer.cpp - -------------------------===// -// ---------------- Can you split out the minimizer -> directives scanner rename into a separate patch? That would help with reviewing. ================ Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:204 - // Pass the skip mappings which should speed up excluded conditional block - // skipping in the preprocessor. - ScanInstance.getPreprocessorOpts() - .ExcludedConditionalDirectiveSkipMappings = &PPSkipMappings; + llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> localDepFS = + DepFS; ---------------- Nit: I think Clang still uses `PascalCase` for variable names. ================ Comment at: clang/test/ClangScanDeps/macro-expansions.cpp:1 +// RUN: rm -rf %t +// RUN: split-file %s %t ---------------- Could you provide a short description of what's the intention of this test? Probably not obvious to the uninitiated. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124687/new/ https://reviews.llvm.org/D124687 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
