[PATCH] D100934: [clang][modules] Build inferred modules
    Duncan P. N. Exon Smith via Phabricator via cfe-commits 
    cfe-commits at lists.llvm.org
       
    Mon May  3 10:37:40 PDT 2021
    
    
  
dexonsmith added a comment.
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?)
Either way, please split the testing between clang's ability to build inferred modules (clang/test/Modules) and clang-scan-deps ability to find them (clang/test/ClangScanDeps).
================
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
----------------
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.
================
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
----------------
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)
Repository:
  rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100934/new/
https://reviews.llvm.org/D100934
    
    
More information about the cfe-commits
mailing list