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 ---------------- 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. 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
