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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a subscriber: urnathan.
dblaikie added a comment.

(I'd probably still reduce the test down to one example, using `-o` and skipping the extra `OUTPUT_PATH` details, only checking the last part of the path is as specified (or perhaps checking that it matches the .o file?))

Also, could you consider the questions @urnathan asked in the cross-project/flag naming thread about the semantics of this flag - I imagine the right behavior is "whatever we do for .o files", but it'd be good to check/consider/test them (they may not need automated testing - if the behaviour is implemented with the same utilities for .o files as for .pcm files then I don't mind just a statement that that's the case and some manual testing has been done to verify that all works)?



================
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
+
----------------
ChuanqiXu wrote:
> 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.
Ah, maybe just testing the explicit `-o` version is adequate? We aren't doing anything interesting here except "whatever the .o path is" - so testing just "-o" seems sufficient to test the one path through this code. The pcm code doesn't have a "-o" and "implicit -o" codepath, just "do whatever the .o path is" so one test seems fine to me?


================
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
+//
----------------
ChuanqiXu wrote:
> 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.
I think the extra precision probably isn't necessary/sufficiently valuable & we could use `{{.*}}` here?


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

https://reviews.llvm.org/D137058