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