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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
