[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked an inline comment as done.
ChuanqiXu added inline comments.


================
Comment at: clang/test/Driver/save-std-c++-module-file.cpp:8-9
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm -DPREFIX=%t --check-prefix=CHECK-WITH-OUTPUT
+
----------------
dblaikie wrote:
> Not sure I understand the need for two tests here - they both specify an absolute path to a .o file & CHECK that the absolute path matches the .pcm output file path, so they don't seem to be testing distinct scenarios?
The above one doesn't specify the path for the .o file. So in the first test the .pcm file will be in the same directory with the input source file. And in the second command line, the .pcm file will be in the same directory with the .o file.


================
Comment at: clang/test/Driver/save-std-c++-module-file.cpp:1-4
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
----------------
dblaikie wrote:
> dblaikie wrote:
> > Is this needed? Maybe we don't need to split the file, if it's just the one file anyway?
> Ping on this ^
Sorry for missing it. Thanks for clarification.


================
Comment at: clang/test/Driver/save-std-c++-module-file.cpp:6
+// RUN: %clang -std=c++20 %t/Hello.cppm -fsave-std-c++-module-file -### 2>&1 | \
+// RUN:   FileCheck %t/Hello.cppm -DPREFIX=%t
+//
----------------
dblaikie wrote:
> Is the prefix needed? I'd expect we'd usually regex match away the actual directory with `{{.*}}` in tests like this?
Yeah, generally we would use `{{.*}}`. And `-DPREFIX` is more precise for what we want to test. So I feel it wouldn't be bad.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137058/new/

https://reviews.llvm.org/D137058