jansvoboda11 added a comment. In D100934#2737130 <https://reviews.llvm.org/D100934#2737130>, @dexonsmith wrote:
> In D100934#2735955 <https://reviews.llvm.org/D100934#2735955>, @jansvoboda11 > wrote: > >> 1. Fix `AsWritten` for umbrella headers/directories. >> 2. Add tests to `clang/test/Modules`. > > It sounds like these two pieces could be done together (since the refactor > for `AsWritten` seems like it deserves a test), as a commit that adds support > for building inferred modules explicitly. > >> 3. Add tests to `clang/test/ClangScanDeps`, make the necessary changes to >> `Tooling/DependencyScanning`, add response file Python script. > > Followed by this, for changing clang-scan-deps to support scanning for > inferred modules. > > WDYT? That sounds good to me. ================ Comment at: clang/test/ClangScanDeps/modules-inferred-explicit-build.m:13-17 +// RUN: %clang @%t.inferred.rsp -pedantic -Werror +// RUN: %clang @%t.system.rsp -pedantic -Werror +// RUN: %clang -x objective-c -fsyntax-only %t.dir/modules_cdb_input.cpp \ +// RUN: -F%S/Inputs/frameworks -fmodules -fimplicit-module-maps \ +// RUN: -pedantic -Werror @%t.tu.rsp ---------------- dexonsmith wrote: > jansvoboda11 wrote: > > dexonsmith wrote: > > > I feel like the clang invocations that build/use modules should be in > > > `clang/test/Modules`. Two independent things: > > > - Can clang build inferred modules explicitly? (tested in > > > clang/test/Modules) > > > - Can clang-scan-deps generate deps for inferred modules? (tested in > > > clang/test/ClangScanDeps) > > I agree that we should test explicit build of inferred modules in > > `clang/test/Modules` (without `clang-scan-deps`). I'll look into it. > > > > I'm not sure I'd be happy with only checking the dependencies produced by > > `clang-scan-deps` here. Testing that the generated command-line actually > > works is as important IMO, and it will be even more so when we start > > optimizing the command line (stripping out unused header search paths, > > definitions etc.). > I'm not sure precisely what you mean by "works". I'm not sure just checking > if it still compiles tells us much. What's important is that the modified > options have the same semantics, and I think a subtle change that still > compiles is more likely than preventing compilation entirely. > > I don't think it would scale to check for semantic problems here -- that > needs a body of testcases that covers all the things modules support. > > One option would be to use the testcases (or a selection of them) in > clang/test/Modules, by adding an extra RUN line that builds > clang-scan-deps-discovered modules both with and without command-line > stripping. For most changes, we can arrange the AST block such that skipped > options won't affect it, and we could compare the hash of just that block. If > and when we start stripping ignored `-D` options we'll need something > smarter, but we can solve that problem later. (Ideally, this would just be a > mode in clang, `clang -Xclang -fmodules-depscan`, which does an initial > depscan and builds the modules in order. This might actually be an > improvement on the existing implicit modules.) > > @Bigcheese, maybe you can weigh in? If you both think `clang -cc1` should be > tested here, I'm open to it. (In that case, though, this should not be > invoking `%clang`, but `%clang_cc1`, I think... or does the response file > create a driver command-line?) > I'm not sure precisely what you mean by "works". I'm not sure just checking > if it still compiles tells us much. What's important is that the modified > options have the same semantics, and I think a subtle change that still > compiles is more likely than preventing compilation entirely. I see, that's a good point. > One option would be to use the testcases (or a selection of them) in > clang/test/Modules, by adding an extra RUN line that builds > clang-scan-deps-discovered modules both with and without command-line > stripping. For most changes, we can arrange the AST block such that skipped > options won't affect it, and we could compare the hash of just that block. If > and when we start stripping ignored `-D` options we'll need something > smarter, but we can solve that problem later. Using tests from `clang/test/Modules` sounds nice. What are your concerns regarding stripping `-D` options? > In that case, though, this should not be invoking `%clang`, but `%clang_cc1`, > I think... or does the response file create a driver command-line? The `Tooling/DependencyScanning` library already prepends the `"-cc1"` argument so that build tools using the API don't have to do that on their own. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100934/new/ https://reviews.llvm.org/D100934 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
