[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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits