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