dblaikie requested changes to this revision. dblaikie added a comment. This revision now requires changes to proceed.
Please update the patch description/commit message to reflect the new naming. ================ 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 + ---------------- 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? ================ Comment at: clang/test/Driver/save-std-c++-module-file.cpp:14-15 + +// CHECK: "-o" "[[PREFIX]]/Hello.pcm" +// CHECK-WITH-OUTPUT: "-o" "[[PREFIX]]/output/Hello.pcm" ---------------- Might be worth fleshing out the CHECKs a bit more - If this patch implements: > Support .cppm -> .pcm + .o in one phase. Then I'd expect the test to demonstrate that - show that there are two commands run by the driver and one outputs the .o file (so include checking for the .o file output file name, and whatever flags tell the frontend that object code is to be emitted) and one that outputs the .pcm file (including checking the .pcm file output file name, and whatever flags tell the frontend that a pcm should be emitted). ================ 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: > Is this needed? Maybe we don't need to split the file, if it's just the one > file anyway? Ping on this ^ 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
