[Lldb-commits] [PATCH] D119831: [lldb] Add support for a "system-wide" lldbinit file

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

This is in line with the existing ways of sourcing init files in LLDB and has all the things we discussed in the RFC. I left few nits but this all looks good to me. I'll hold off on accepting so it continues to show up in the other reviewer's queue.



================
Comment at: lldb/include/lldb/Interpreter/CommandInterpreter.h:256
   void SourceInitFileHome(CommandReturnObject &result, bool is_repl);
+  void SourceSystemInitFile(CommandReturnObject &result);
 
----------------
`SourceInitFileSystem` for consistency with the other two? Or maybe you were planning to change the other two in another patch?


================
Comment at: lldb/source/API/SBCommandInterpreter.cpp:420-422
+void SBCommandInterpreter::SourceSystemInitFile(SBCommandReturnObject &result) {
+  LLDB_INSTRUMENT_VA(this, result);
+  result.Clear();
----------------
Nit: all the other functions //should// have a newline because that's what `lldb-instr` generates.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2385-2393
+#ifdef LLDB_SYSTEM_INIT_PATH
+    FileSpec init_file(LLDB_SYSTEM_INIT_PATH);
+    if (init_file) {
+      init_file.MakeAbsolute(HostInfo::GetShlibDir());
+    }
+
+    SourceInitFile(init_file, result);
----------------
Why not put the ifdef around the `m_skip_lldbinit_files` check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119831