abrachet marked 2 inline comments as done.
abrachet added inline comments.
================
Comment at: clang/lib/Driver/Driver.cpp:1434
StringRef AdditionalInformation, CompilationDiagnosticReport *Report) {
+#ifdef CLANG_CRASH_SAVE_TEMPS
+ constexpr bool SaveReproTemps = true;
----------------
arichardson wrote:
> This could be simplified with `#cmakedefine01` in the config header
Thanks I like that a lot better.
================
Comment at: clang/test/Driver/crash-report-clang-cl.cpp:1
+// UNSUPPORTED: windows
+
----------------
arichardson wrote:
> I think it would be cleaner to add a new feature if tar is available in $PATH
> instead of ignoring all windows bots. Some might have tar installed.
It's not clear to me what the Windows builders don't like. `tar(1)` is used in
some lld tests. My guess is that the tar shipped on Windows doesn't support
`--wildcards` which is seemingly necessary here because the tar file will have
a random name, and it's member files are prefixed with that name. I don't have
easy access to a Windows machine to test that theory though.
I've gone with your suggestion to have an env var. Do you think I should remove
`UNSUPPORTED: windows` on all tests or just this one?
================
Comment at: clang/test/Driver/crash-report-clang-cl.cpp:8
// RUN: -fcrash-diagnostics-dir=%t -- %s 2>&1 | FileCheck %s
-// RUN: cat %t/crash-report-clang-cl-*.cpp | FileCheck --check-prefix=CHECKSRC
%s
-// RUN: cat %t/crash-report-clang-cl-*.sh | FileCheck --check-prefix=CHECKSH %s
+// RUN: tar xOf %t/*.tar --wildcards "*/tmp/crash-report-clang-cl-*.cpp" \
+// RUN: | FileCheck --check-prefix=CHECKSRC %s
----------------
arichardson wrote:
> Alternatively, if we had an env var/command line option to keep the cpp/sh,
> we could use that to avoid the dependency on tar in this test.
Sure.
================
Comment at: clang/test/Driver/crash-report-save-temps.c:14
+
+// RUN: false
----------------
arichardson wrote:
> Is this line intentional?
Nope, thanks.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122335/new/
https://reviews.llvm.org/D122335
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
