[PATCH] D122335: [clang] Emit crash reproduction as a single tar file

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added inline comments.


================
Comment at: clang/CMakeLists.txt:520
+                       ON "${CMAKE_BUILD_TYPE} MATCHES Debug" OFF)
+if(CLANG_CRASH_SAVE_TEMPS)
+  add_definitions(-DCLANG_CRASH_SAVE_TEMPS)
----------------
Could you add this to the config.h header instead?


================
Comment at: clang/lib/Driver/Driver.cpp:1434
     StringRef AdditionalInformation, CompilationDiagnosticReport *Report) {
+#ifdef CLANG_CRASH_SAVE_TEMPS
+  constexpr bool SaveReproTemps = true;
----------------
This could be simplified with `#cmakedefine01` in the config header


================
Comment at: clang/test/Driver/crash-report-clang-cl.cpp:1
+// UNSUPPORTED: windows
+
----------------
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.


================
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
----------------
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.


================
Comment at: clang/test/Driver/crash-report-save-temps.c:14
+
+// RUN: false
----------------
Is this line intentional?


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

https://reviews.llvm.org/D122335