[Lldb-commits] [PATCH] D134991: [lldb] Add diagnostics

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

I like the direction, thanks for working on this.

To no one's surprise, I was thinking about testing :)

- Errors related to the filesystem e.g. can't open a path, is too complicated to setup on demand. The code looks to be passing on the error, which is good enough most of the time.
- The callbacks are added in c++, so testing if you add A then remove B and so on doesn't make sense on the same copy of lldb (and upstream will always have the same set of callbacks anyway).
- Dumping statistics uses an existing method, you just make up the filename. So it's the statistics code's responsibility to test what that function does.

Maybe there could be a smoke test that just checks that the dir is made and has some files ending in `stats.json` in it? I think clang does something like this though they may have a `--please-crash` option too.

Unrelated - there's maybe a situation where the same set of callbacks are added in a different order, perhaps? But am I correct in thinking that this isn't an issue because these will be writing to different files? Stats has one set of files, logging has another set, etc. So the end result is always a dir of separate files one or more for each thing that registers.



================
Comment at: lldb/include/lldb/Utility/Diagnostics.h:29
+public:
+  Diagnostics();
+  ~Diagnostics();
----------------
Can this be private? Code should only use `Initialize` if I am reading this right.


================
Comment at: lldb/include/lldb/Utility/Log.h:231
                    llvm::StringRef function, const char *format,
-                   Args &&... args) {
+                   Args &&...args) {
     Format(file, function,
----------------
These seem unrelated but not doing any harm.


================
Comment at: lldb/source/API/SBDebugger.cpp:222
 
+static void DumpDiagnostics(void* cookie) {
+  Diagnostics::Instance().Dump(llvm::errs());
----------------
What is `cookie` used for? (or rather, used elsewhere)


================
Comment at: lldb/source/Utility/Diagnostics.cpp:43
+  std::lock_guard<std::mutex> guard(m_callbacks_mutex);
+  m_callbacks.push_back(callback);
+}
----------------
Is it worth adding an assert that the callback is not already in the list?

(and below, that the callback is in fact in the list)


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

https://reviews.llvm.org/D134991