[PATCH] D74324: Tools emit the bug report URL on crash

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:1272
-         "crash backtrace, preprocessed source, and associated run script.";
-
   // Suppress driver output and emit preprocessor output to temp file.
----------------
probinson wrote:
> No, for clang crashes we want the message to say all these things. The driver makes an effort to produce preprocessed source and a run script; it's hard to diagnose a crash without them.
@probinson - what are your thoughts on the latest version? Note that the output style is slightly different from before, but the message is now the same.


================
Comment at: llvm/include/llvm/Support/PrettyStackTrace.h:42
+  /// a crash.
+  void SetCustomBugReportMsg(const char *msg);
+
----------------
`msg` -> `Msg`
`SetCustomBugReportMsg` -> `setBugReportMsg`


================
Comment at: llvm/lib/Support/PrettyStackTrace.cpp:145
 
+static const char* customBugReportMsg;
+
----------------
`static const char *BugReportMsg;`

Why not just have this set to the default in the first place, and overwrite it if somebody needs to? That'll remove the need for the new `if` in the `CrashHandler` function.


================
Comment at: llvm/lib/Support/PrettyStackTrace.cpp:147
+
+void llvm::SetCustomBugReportMsg (const char* msg) {
+    customBugReportMsg = msg;
----------------
No need for the explicit `llvm::` - you are inside a `using namespace llvm` file.

Also `msg` -> `Msg`.


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

https://reviews.llvm.org/D74324