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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
