[PATCH] D28105: [DWARF] - Introduce DWARFCompression class.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFCompression.h:21
+/// This class helps to handle decompression of compressed debug sections.
+class DWARFCompression {
+public:
----------------
dblaikie wrote:
> Slightly odd/non-descript name (DWARFDecompressor? it seems specifically for decompressing)
Renamed to Decompressor, since it is lib/Object/Decompressor now


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFCompression.h:23
+public:
+  DWARFCompression(StringRef Name, StringRef Data, bool IsLE, bool Is64Bit);
+
----------------
aprantl wrote:
> Please comment the parameters here.
Done.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFCompression.h:29
+  /// Methods uncompresses section data to raw buffer provided.
+  bool uncompress(char *Buffer, size_t Size);
+
----------------
aprantl wrote:
> MutableArrayRef?
Done.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFCompression.h:31-32
+
+  /// Used to get text representation of error if any happened.
+  StringRef getErrorMessage() { return ErrorMsg; }
+
----------------
dblaikie wrote:
> Could uncompress return Error instead of bool, and then the error message could be included there?
Done.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFCompression.h:37
+
+  /// Using section name and flags returns if section is a ELF compressed one.
+  static bool isCompressedELFSection(uint64_t Flags, StringRef Name);
----------------
davide wrote:
> Use an uniform style for comments. Also, doxygen.
Done.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFCompression.h:40
+
+  /// Returns if section name is matches gnu style compressed one.
+  static bool isGnuStyle(StringRef Name);
----------------
davide wrote:
> Returns what? Probably you mean returns true?
Done.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFCompression.h:43-44
+
+  /// Contains memory buffer size required for decompression.
+  uint64_t UncompressedSize;
+
----------------
dblaikie wrote:
> strange to have a mutable public member - should this be an accessor?
Made an accessor.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:608
       SmallString<32> Out;
-      if (!tryDecompress(name, data, Out, ZLibStyleCompressed, IsLittleEndian,
-                         AddressSize == 8))
+      if (!Decompressor.uncompress(Out))
         continue;
----------------
davide wrote:
> decompress instead of uncompress maybe?
Done.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:614
 
+    name = name.substr(
+        name.find_first_not_of("._z")); // Skip ".", "z" and "_" prefixes.
----------------
davide wrote:
> this deserves a comment or needs to be hidden behind and API
Added comment for ".z". 

Note that line comes from original code, I just moved it below and added "z" handling. 
Not sure why originally "_" was also dropped, 
I never saw sections with name like "._xxx". 
So I commented only my new logic for dropping "z".


https://reviews.llvm.org/D28105