[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This is looking much better -- and focused. I still have comments though. :)



================
Comment at: lldb/include/lldb/Core/Module.h:833
 
-  void LogMessageVerboseBacktrace(Log *log, const char *format, ...)
-      __attribute__((format(printf, 3, 4)));
+  void LogMessageVerboseBacktrace(Log *log, const char *format, ...);
 
----------------
Could you do this one as well? It only has one caller and it would be strange for it to use a different syntax than LogMessage


================
Comment at: lldb/include/lldb/Core/Module.h:837
+  void ReportWarning(const char *format, Args &&...args) {
+    if (format && format[0])
+      ReportWarning(llvm::formatv(format, std::forward<Args>(args)...));
----------------
Let's remove this defensive nonsense while we're in here. All of the callers are in this patch, and none of them is passing an empty string or a null pointer.


================
Comment at: lldb/include/lldb/Utility/Status.h:65
+  template <typename... Args>
+  explicit Status(const char *format, Args &&...args) {
+    SetErrorToGenericError();
----------------
I don't think you've converted all the callers of this one. Given it's pervasiveness, I think we will need to do some sort of a staged conversion, to ensure call sites get compile errors instead of silent breakage. For now I think it would be fine to have a "named constructor" using formatv (`static Status Status::WithFormat(fmt, args...)`). Or just leave it out...


================
Comment at: lldb/source/Core/Module.cpp:1157
       m_first_file_changed_log = true;
-      if (format) {
-        StreamString strm;
-        strm.PutCString("the object file ");
-        GetDescription(strm.AsRawOstream(), lldb::eDescriptionLevelFull);
-        strm.PutCString(" has been modified\n");
-
-        va_list args;
-        va_start(args, format);
-        strm.PrintfVarArg(format, args);
-        va_end(args);
-
-        const int format_len = strlen(format);
-        if (format_len > 0) {
-          const char last_char = format[format_len - 1];
-          if (last_char != '\n' && last_char != '\r')
-            strm.EOL();
-        }
-        strm.PutCString("The debug session should be aborted as the original "
-                        "debug information has been overwritten.");
-        Debugger::ReportError(std::string(strm.GetString()));
-      }
+      m_first_file_changed_log = true;
+      StreamString strm;
----------------
duplicated line


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:21-31
+  s->Printf(
+      "%s",
+      std::string(
+          llvm::formatv(
+              "{0:x+16}: Compile Unit: length = {1:x+8}, version = {2:x}, "
+              "abbr_offset = {3:x+8}, addr_size = {4:x+2} (next CU at "
+              "[{5:x+16}])\n",
----------------



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:197
               "attach the file at the start of this error message",
-              m_offset, (unsigned)form);
+              (uint64_t)m_offset, (unsigned)form);
           *offset_ptr = m_offset;
----------------
this shouldn't be necessary


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFTypeUnit.cpp:18-27
+  s->Printf("%s",
+            std::string(
+                llvm::formatv(
+                    "{0:x+16}: Type Unit: length = {1:x+8}, version = {2:x+4}, "
+                    "abbr_offset = {3:x+8}, addr_size = {4:x+2} (next CU at "
+                    "[{5:x+16}])\n",
+                    GetOffset(), (uint32_t)GetLength(), GetVersion(),
----------------
same here


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:585-586
+                                  "DW_AT_rnglists_base for CU at {0:x+16}",
+                                  GetOffset()))
+            .c_str());
   if (std::optional<uint64_t> off = GetRnglistTable()->getOffsetEntry(
----------------
This should be enough (also, I think doing a .str() on the format result would look nicer than a std::string constructor)


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3031
             log,
+
             "SymbolFileDWARF::"
----------------
delete empty line


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:11-58
+#include "AppleDWARFIndex.h"
+#include "DWARFASTParser.h"
+#include "DWARFASTParserClang.h"
+#include "DWARFCompileUnit.h"
+#include "DWARFDebugAbbrev.h"
+#include "DWARFDebugAranges.h"
+#include "DWARFDebugInfo.h"
----------------
ayermolo wrote:
> JDevlieghere wrote:
> > Unrelated change?
> This was recommended in another diff.  Although it was related to DIERef.h. I can move it to the second diff, or it's own diff?
Yeah, this looks messy if the clang-format layout is significantly different than the original. Maybe you shouldn't do it here -- or do it as a separate patch (but do keep it in mind for future, particularly when creating new files).


================
Comment at: lldb/source/Symbol/DWARFCallFrameInfo.cpp:775-784
+            LLDB_LOGF(
+                log, "%s",
+                std::string(
+                    llvm::formatv("DWARFCallFrameInfo::{0}(dwarf_offset: "
+                                  "{1:x+16}, startaddr: [{2:x+16}] encountered "
+                                  "DW_CFA_restore_state but state stack "
+                                  "is empty. Corrupt unwind info?",
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139955