[Lldb-commits] [PATCH] D51578: 02/03: Contiguous sections (.debug_info+.debug_types) for D54670==D32167 (.debug_types)

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Few comments inline.



================
Comment at: lldb/include/lldb/Symbol/ObjectFile.h:781
+    friend std::unique_ptr<SectionReader> ObjectFile::SectionReaderFactory(
+	const SectionPart &section_part);
+    SectionReader(const SectionPart &section_part_);
----------------
Please clang-format the patch before landing this. 


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3400
+  SectionReaderCompressed(const SectionPart &section_part_);
+  uint64_t getDecompressedSize() override;
+  size_t read(DataExtractor &section_data) override;
----------------
Rather than returning `0` when there's no decompressor, why not return llvm::Optional<uint64_t> to differentiate between with an actual size of zero? 


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3409
+  // 'decompressor' Initialize() requires already initialized 'compressed_data'.
+  llvm::Expected<llvm::object::Decompressor> decompressor;
+};
----------------
I don't think having an expected as a member makes sense. Maybe you want to use an optional? Is the goal to abstract over the decompression not being available? Or should the class assume the the decompressor is there (and itself be wrapped in an expected when that's not the case?). I'm not too familiar with this code but it seems a rather odd design. 


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+ObjectFileELF::SectionReaderCompressed::SectionReaderCompressed(
+    const SectionPart &section_part_)
+    : ObjectFile::SectionReader(section_part_),
----------------
Trailing underscore?


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3467
+  if (retval == 0)
+    section_data.Clear();
+  else {
----------------
You could early return here (`return 0;`). 


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:684
+    // present without .debug_info as that should not happen.
+    if (!m_obj_file->IsInMemory() && debug_info_part
+        && !debug_info_part.GetSection()->Test(llvm::ELF::SHF_COMPRESSED)
----------------
Can we split this up? There's no way I remember the first two clauses by the time I'm reading the third. 


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D51578