[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]

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