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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added a comment.
This revision now requires changes to proceed.

I don't really understand the motivation. Can you elaborate on why "enabling 64 bit support" requires this change? I definitely prefer the `formatv` approach, but wouldn't the PRIx64 should do what you want here?

Also, at a higher level, can we do the same thing as the `Log::Format` function that takes varargs and uses `formavt` under the hood?

  template <typename... Args>
  void Format(llvm::StringRef file, llvm::StringRef function,
              const char *format, Args &&... args) {
    Format(file, function, llvm::formatv(format, std::forward<Args>(args)...));
  }



================
Comment at: lldb/include/lldb/Core/Module.h:827-837
   void LogMessage(Log *log, const char *format, ...)
       __attribute__((format(printf, 3, 4)));
 
   void LogMessageVerboseBacktrace(Log *log, const char *format, ...)
       __attribute__((format(printf, 3, 4)));
 
   void ReportWarning(const char *format, ...)
----------------
Let's not keep both. 


================
Comment at: lldb/include/lldb/Core/Module.h:839
 
+  // The llvm::formatv version of above APIs
+  void LogMessage(Log *log, const std::string &msg);
----------------
This should be (1) a doxygen comment and (2) using a group. But that's moot if we don't keep the printf-style variants around. 


================
Comment at: lldb/source/Core/Module.cpp:1248-1253
+void Module::LogMessage(Log *log, const std::string &msg) {
+  StreamString log_message;
+  GetDescription(log_message.AsRawOstream(), lldb::eDescriptionLevelFull);
+  log_message.PutCString(": ");
+  log->PutCString(log_message.GetData());
+}
----------------
This is not using `msg`. Missing `strm.PutCString(msg);`? 


================
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"
----------------
Unrelated change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139955