[PATCH] D124687: [Tooling/DependencyScanning & Preprocessor] Refactor dependency scanning to record and use pre-lexed preprocessor directive tokens, instead of minimized sources

Jan Svoboda via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun May 1 23:43:03 PDT 2022

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list