[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

Overall seems good to me. Minor comments inline.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:27
                                          lldb::offset_t *offset_ptr) {
+  llvm::DataExtractor llvm_data = data.GetAsLLVM();
   const lldb::offset_t begin_offset = *offset_ptr;
----------------
Is this intentionally a copy or should this be a reference? I have no idea how heavyweight this object is...


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp:56
   if (m_idx_offset == UINT32_MAX) {
-    DWARFAbbreviationDeclarationCollConstIter pos;
-    DWARFAbbreviationDeclarationCollConstIter end = m_decls.end();
-    for (pos = m_decls.begin(); pos != end; ++pos) {
-      if (pos->Code() == abbrCode)
-        return &(*pos);
+    for (const auto &decl : m_decls) {
+      if (decl.getCode() == abbrCode)
----------------
would std::find_if be shorter or would it look worse?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h:18
 
+#include "llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h"
+
----------------
Nit: we usually do local includes first and then the stdlib at the bottom.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150716