jansvoboda11 added a comment.
In D100934#2733857 <https://reviews.llvm.org/D100934#2733857>, @dexonsmith
wrote:
> Given that there are four different things being done in this commit, it
> sounds naively like it should be four separate commits. If the logic is too
> intertwined to land each of the four pieces separately (certainly possible),
> can you quickly explain why, to motivate landing them together? (Or maybe
> there's an NFC refactoring that could be separated as a prep, to make the
> functional changes more isolated / easy to reason about?)
I think that landing everything together provides more context for people going
through git log in the future.
If you really think splitting this up would be better, how about this?
1. Fix `AsWritten` for umbrella headers/directories.
2. Add tests to `clang/test/Modules`.
3. Add tests to `clang/test/ClangScanDeps`, make the necessary changes to
`Tooling/DependencyScanning`, add response file Python script.
================
Comment at: clang/test/ClangScanDeps/modules-inferred-explicit-build.m:10-12
+// RUN: %S/module-deps-to-rsp.py %t.db --module-name=Inferred > %t.inferred.rsp
+// RUN: %S/module-deps-to-rsp.py %t.db --module-name=System > %t.system.rsp
+// RUN: %S/module-deps-to-rsp.py %t.db --tu-index=0 > %t.tu.rsp
----------------
dexonsmith wrote:
> I took me a while to figure out that `.rsp` and `-to-rsp` means "response
> file". Can we just use `response` (or even `response-file`?) in both cases,
> or is `.rsp` a well-known extension I'm just not aware of?
>
> I'm also not sure we want to use this in a test (see my next comment). If
> it's a useful utility regardless perhaps it could have a home in clang/utils.
The Clang driver already has an option `--rsp-quoting={posix,windows}`, so I
think the shortcut is somewhat known. I can rename the script to
`module-deps-to-response-file`, if you think that's clearer. It could be useful
in general, co moving this to `clang/utils` sounds fine.
================
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:
> 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.).
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
