-
Notifications
You must be signed in to change notification settings - Fork 353
Merge commit 'b70be3dc14c1' from llvm.org/main into next #11943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge commit 'b70be3dc14c1' from llvm.org/main into next #11943
Conversation
…ndencyScanningTool (NFC) (llvm#169962) This patch is the first of two in refactoring Clang's dependency scanning tooling to remove its dependency on clangDriver. It separates Tooling/DependencyScanningTool.cpp from the rest of clangDependencyScanning and moves clangDependencyScanning out of clangTooling into its own library. No functional changes are introduced. The follow-up patch (llvm#169964) will restrict clangDependencyScanning to handling only -cc1 command line inputs and will move all functionality related to handling driver commands into clangTooling. (Tooling/DependencyScanningTool.cpp). This is part of a broader effort to support driver-managed builds for compilations using C++ named modules and/or Clang modules. It is required for linking the dependency scanning tooling against the driver without introducing cyclic dependencies, which would otherwise cause build failures when dynamic linking is enabled. The RFC for this change can be found here: https://discourse.llvm.org/t/rfc-new-clangoptions-library-remove-dependency-on-clangdriver-from-clangfrontend-and-flangfrontend/88773?u=naveen-seth
befb44d to
60377ab
Compare
|
Hey @jansvoboda11 @Bigcheese, could you review this PR when you get a chance? |
jansvoboda11
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I already had this resolved locally, but your version helped me to catch some issues on my side (dropped test, missed simplification, etc.) I combined our changes and pushed to the automerger ref.
| #include "clang/DependencyScanning/DependencyScanningWorker.h" | ||
| #include "clang/DependencyScanning/ScanAndUpdateArgs.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DependencyScanningWorker.h isn't necessary and ScanAndUpdateArgs.h can be moved into IncludeTreeActionController.cpp.
| #include "clang/Frontend/CompilerInvocation.h" | ||
| #include "clang/Lex/HeaderSearchOptions.h" | ||
| #include "clang/Tooling/DependencyScanning/DependencyScanningTool.h" | ||
| #include "clang/Tooling/DependencyScanningTool.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong, introduces dependency from DependencyScanning to Tooling. This only needs to include clang/DependencyScanning/ModuleDepCollector.h.
|
Glad it helped, and thanks for the review! I’ll be sure to ping beforehand next time if I can also prepare a downstream PR. |
llvm#169962 separates the
DependencyScanningToolfrom the rest ofclangDependencyScanningand movesclangDependencyScanningout ofclangToolinginto its own library.This resolves the merge conflict with Swift’s downstream changes to the dependency-scanning tooling and properly integrates the upstream PR into swift-lang/llvm-project.