dexonsmith added inline comments.
================ 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: > > > 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. > > > > Using tests from clang/test/Modules sounds nice. What are your concerns > > regarding stripping -D options? > > Oh, it’s a bit mundane, but that’ll affect the identifier info, which will in > turn change the AST block and its hash. I think? > > For a truly explicit module, removing “unused” `-D` options from the > identifier table might not be correct, since for semantics you might want > importers to pick up those definitions (I think? Are they considered > exported?). For implicitly-discovered modules, we know all transitive > importers have a superset of `-D`s on their command-lines so it’s safe. > > Maybe what we can do at the time is turn on a flag to avoid writing unused > `-D`s to the serialized identifier table so the hashes will match? Anyway, > I’m sure there is a solution. > > 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. I suggest adding a `.cc1` extension to the response files to make that clear to anyone reading the test. Maybe `t.inferred.cc1.rsp`. The third clang invocation doesn't have the response file as the first argument, though, so I imagine that one doesn't contain a `-cc1`? (Or if it did, I'm not sure how it would work... I'm guessing it only adds arguments that happen to match between `-cc1` and driver?) 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
